> >> +// how many strings allowed in command line (AHanses)
> >> +#ifndef MAX_ARGV
> >> +#define MAX_ARGV 255
> >> +#endif
> >
> >We don't like #define's. Is this really necessary?
>
> The size of the argument vector should be synchronized with the
> OS-default value for this. How to best achieve this in a portable way?
use
int const max_argv = 255;
instead. This is typesafe, the name is entered into the symbol table,
and thus, it will appear when you debug.
> >> #include <unistd.h>
> >> +#include <process.h>
> >> #include "syscall.h"
> >
> >I don't this we need this for Unix.
>
> Does it hurt? If it hurts, we have to add another ugly #ifdef Blurb.
No.
> >> if (pid>0) { // Fork succesful. Wait for child
> >> +#ifndef __EMX__
> >> waitForChild();
> >> +#else
> >> + retval = pid;
> >> +#endif
> >
> >Would it be possible to use the alternative that waitForChild is
> >different on EMX?
>
> Possible, yes! But why?
Because waitForChild might be used several places, and then we will
only need one #ifdef guard.
> >> // Wait for child process to finish. Returns returncode from child.
> >> +#ifndef __EMX__
> >> void Systemcalls::waitForChild()
> >> {
> >
> >> }
> >> +#endif
> >
> >See above: Instead of dropping this function, please just make
> >the waitForChild operation a dummy operation.
>
> No problem, but why bloat classes with dummy members? (IMHO the LyX
> classes are bloated, I guess you really need some 60% of the puublic
> members)
The method is not dummy on Unix. It's better to have the same structure
across platforms, even if the implementation is different (or missing.)
> >> - const int MAX_ARGV = 255;
> >
> >If the problem is that MAX_ARGV might be defined already in OS/2,
> >I think the better solution is to rename this local constant.
>
> I think it might be better to catch the OS' default value.
It's still possible:
#ifdef MAX_ARGV
int const max_argv = MAX_ARGV;
#else
int const max_argv = 255;
#endif
> >> +/** AHanses: Eliminate unneeded parameter for Unix: 'fork(void)'
> >> + Unix will just ignore the spawnFlag parameter
> >> +*/
> >
> >This comment should go. The remark that Unix should ignore the
> >spawnFlag should go as a comment near the Fork-method.
>
> No problem. This was mostly for me. Nevertheless one personal note
> here:
> When reading sources originally written for OS/2 and compare them with
> the usual hacks from bsd or GNU I see one important difference:
>
> The comments / code ratio is normally 2 / 1 or even 3 / 1 with broad
> coverage of design, implementation, security, preconditions,
> postconditions, restrictions, etc. It seems the usual Unix hacker
> thinks he does not need to extensively comment out the source to avoid
> design and implementation bugs. Why?
The comment you wrote is placed the wrong place.
Also, please read the SystemCalls header file in cvs. It's richly
commented.
> >Finally, the patch should be remade against the latest cvs version.
>
> I think I better let you do this, otherwise I might introduce more
> bugs.
If you do it, I'll review the patch before we apply it. I don't have
time to redo the patch, and I can't possibly test it on OS/2.
> Please note that signals often are a portability headache, especially
> when sending them to other processes. Did you investigate how this
> works, e.g. in cygwin?
No I did not. As with all other system code in LyX: We write it first,
and then we learn about the problems with it when it is deployed on
different platforms. This is the most cost effective way to go, and
is the best way to take advantage of the open source development model
where we have many testers.
Greets,
Asger
