[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [cygwin] cwrapper emits wrapper script

From: Ralf Wildenhues
Subject: Re: [cygwin] cwrapper emits wrapper script
Date: Tue, 24 Apr 2007 08:53:46 +0200
User-agent: Mutt/1.5.15 (2007-04-13)

* Charles Wilson wrote on Tue, Apr 24, 2007 at 04:34:41AM CEST:
> Ralf Wildenhues wrote:
>> (note I'm not asking you to do this work here; actually, I'd like to ask
>> you not to fix even more different things with one patch.  Merely noting
>> it in case you're interested.)
> Ok, but it my defense: NOT fixing the mingw case (e.g. leaving the 
> wrapper.exe broken on mingw) + eliminate the wrapper script from '.' == 
> break EVERYTHING on mingw.  So, I kinda HAD to address this: and two 
> patches that touch exactly the same region of code is rather silly.

Certainly.  I was merely trying to not infer that you'd have to do even
more work than the lot that you're already doing.  Of course if you're
ambitious go for it.  ;-)

Thanks for fixing the MinGW case here.

>> Hmm, maybe one after the `rm -f "$prefix/bin/..."' and one after the
>> `$MAKE uninstall' line?
> Tabled.  Don't want to tackle more than one thing in a patch, you know. <g>

Yes, but that particular two-line change could go in before everything
else, given that it fixes the failure.

>> (Not sure about this WRAPPER_SCRIPT_BELONGS_IN_OBJDIR thing yet.)
> What's not to like?  A wrapper script that lives in $objdir *must* behave 
> ever-so-slightly differently from a wrapper script that lives in $objdir/.. 
> -- they must compute the location of the target executable. Couple that 
> with wanting to use the same func_emit_wrapper_script() method in both 
> cases...

Yes, yes.  I was thinking out loud and it was late...

>>> -# define DEBUG(format, ...)
>>> +# define LTWRAPPER_DEBUGPRINTF(format, ...)
>> What's the reason for this change?
> Because I've always felt really stupid about my original choice: using the 
> incredibly common "DEBUG" symbol as a function-like macro? What was I 
> smoking?

Hmm.  OK.  But since you undefine DEBUG anyway (and you've kept that for
the changed macro), there was no underlying problem here, no?  Anyway,
keeping this change is fine.

>> or by multiple -e.  Also, after a pipe (|), there's no need to escape
>> the newline.
>>> +       $ECHO ";"
>> Please use plain `echo' here.  $ECHO is for stuff that can contain \t,
>> begin with -n, and the like.
> oookay.

This is because $ECHO can be much much more expensive than echo, forking
and all.  Not on w32 especially (unless you happen to use a shell such
as dash), but in general, yes.

>>> +    if (strncmp(argv[i], dumpscript_opt, dumpscript_len) == 0)
>> Please use strcmp, and drop dumpscript_len.
> okay.  But don't come crying to me if somebody populates your argv[] with 
> non-null-terminated strings.

How would that be possible?  ISO C guarantees them to be strings, i.e.,
null-terminated runs of characters.

>> Probably for the cross-compile + simulator case we'd need to allow for
>> some override.  Not sure if we want to factor out the path translation
>> (also not sure if we couldn't just use $fix_srcfile_path for this, and
>> not distinguish between cygwin and mingw at all, but that would be a
>> more far-reaching change).
> A halfway step:
> if test -n "$TARGETSHELL" ; then
>   # no path translation at all
>   lt_newargv1="$TARGETSHELL"
> else
>   esac
> fi
> << now use $lt_newargv1 unconditionally >>

Sounds viable.  (picky note: on the right hand side of a shell
assignment, there is no need for double quotes, as word splitting is
disabled there.)

>> [...]
>>> +  {
>>> +     char* p = newargz[1];
>>> +     while(*p!='\0') {
>>> +       if (*p == '\\') {
>> strchr?
>>> +         *p = '/';
>>> +       }
>>> +       p++;
>>> +     }
>>> +  }
> Thought about it.  But (a) I'd still need the while loop, because I need to 
> replace all '\\' chars, and (b) what else does strchr DO but iterate thru 
> the string?

Iterate faster.

> may be a few lines longer, but avoids an extra function call...

The compiler may be smart enough to inline the function call.  Of course
it may be smart enough to detect that you're emulating strchr, and
insert its faster version there.  But anyway Let's not try to be clever
here by doing micro-optimization which potentially outsmarts the
compiler/C library.  I think using strchr is clearer.

>>> +  if ( (shwrapperFILE = fopen (newargz[1], "wb")) == 0 )
>> Why binary?
> On cygwin, with latest bash, you have to jump thru hoops (set -igncr, use 
> text mounts, etc) to execute scripts that have \r in them, whereas '\n' 
> without '\r' will always work -- even on msys, even on cygwin 'text 
> mounts'.  So, "wt" is *definitely* out.  That leaves "w" which is a very 
> tempting target for someone to "optimize" into "wt" (scripts are text, 
> right?).  So it was either "wb" which always works, or
>        /* note: do NOT use "wt" here! -- defer to underlying
>         * mount type on cygwin
>         */
>        if ( (shwrapperFILE = fopen (newargz[1], "w")) == 0 )

OK, thanks for the explanation.

>>>    for (i=0; i<argc+1; i++)
>>>    {
>>> -    DEBUG("(main) newargz[%d]   : %s\n",i,newargz[i]);
>>> +    LTWRAPPER_DEBUGPRINTF("(main) newargz[%d]   : %s\n",i,newargz[i]);
>>>      ;
>> What's that extra ; for BTW?
> If !DEBUGWRAPPER, then LTWRAPPER_DEBUGPRINTF() goes away completely, and 
> gcc complains about the empty body in the for loop.

D'oh.  Thanks.

>>> +            /* search backwards for last DIR_SEPARATOR */
>>> +            p = tmp_pathspec + strlen(tmp_pathspec) - 1;
>>> +            while ( (p > tmp_pathspec) && (! IS_DIR_SEPARATOR(*p)) )
>>> +                p--;
>> strrchr?
> Nope, can't do that here.  IS_DIR_SEPARATOR(c) checks whether c is '\\' OR 
> '/'; trying to use strrchr in that situation is more confusing than it is 
> worth.

Ah, ok.  Sorry.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]