libtool-patches
[Top][All Lists]
Advanced

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

Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.


From: Charles Wilson
Subject: Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.
Date: Wed, 07 May 2008 00:20:33 -0400
User-agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.14) Gecko/20080421 Thunderbird/2.0.0.14 Mnenhy/0.7.5.666

Eric Blake wrote:
According to Charles Wilson on 5/5/2008 6:23 PM:
| 2008-05-05  Charles Wilson  <...>
|
|     * libltdl/config/ltmain.m4sh (func_to_native_path):

I've become accustomed to using a 1-line summary in my ChangeLog messages
prior to the first '* file:' line; that way, the summary can be reused as
the git commit summary.

Actually, my commit message has that, but when I edited the email for sending, I deleted it.

|
| I'm sorta thinking I should rename the func_to_native* functions
s/native/host/ ?

Yes, that would be appropriate.

Done.

I'd like to see that, and other cleanups
below, before approving.  I haven't tested yet, but the concepts look sane
by inspection.

Some pretty long lines; is it worth trying to wrap at 80 columns?

Wrapped most of them so that they were < 80, or at least much closer
to 80. With these long variable names, it's difficult, and I don't really want to shorten them. The whole point of naming all "local" variables after the function itself is to prevent variable collision. But it makes for long varnames and long lines.

Doesn't func_error die when called?  If so, how do the subsequent three
errors get printed?  If not, the name seems a bit misleading...

See Gary's reply.

| +            func_to_native_pathlist_tmp1="$1"
| +            func_to_native_pathlist_tmp2=`echo
"$func_to_native_pathlist_tmp1" | $SED -e 's|^:*||'`
| +            func_to_native_pathlist_tmp1=`echo
"$func_to_native_pathlist_tmp2" | $SED -e 's|:*$||'`

Avoid some forks - can't you combine this into one sed invocation?
Similar comment about long lines.

I made the change, but I haven't yet tested it. In one of these patches, there was a /reason/ some $SED chain had to be done as separate steps, but it might not be /this/ $SED chain...

It's a bit of a shame that we can't rely on getopt_long and get option
abbreviations; oh well.

That's a feature, not a bug. I don't want the user to have to remember that he must use '--' if his application wants '-e'. Long option names, in the --lt-* namespace == not likely to collide with the target app's option set.

| -        cat <<EOF
| +        cat <<"EOF"

I find 'cat <<\EOF' easier to type (one less character); but the result is
the same; if any part of EOF is quoted in any fashion, the entire here-doc
is parsed without expansion.

Ah, but my syntax highlighter marks "EOF" as a bright red string. \EOF looks just like EOF.

| +  target_name = (char *) xstrdup (base_name (actual_cwrapper_path));
>
Why the cast? Shouldn't xstrdup output char* already?

Yes, you're right. target_name is a renamed versions of shwrapper_name, and it used the cast. Dunno why, but I didn't change it. Fixed now.


Also, gnulib's
base_name mallocs; if we ever make libtool's base_name match, then this
would leak memory (ie. xstrdup of gnulib's base_name is wasteful).  But
your patch didn't affect base_name.

Well, yeah; if you change the behavior of the base_name() included in the cwrapper_src, you better audit the uses of that function within the cwrapper_src.


| +          if (i+1 < argc)

Coding style: s/i+1/i + 1/

Ack.

| +            {
| +              lt_opt_process_env_set (argv[i+1]);
| +              i++; /* don't copy */
| +            }
| +          else
| +            {
| +              lt_fatal ("%s missing required argument", env_set_opt);
| +            }

Coding style: a single expression inside a control block does not need braces.

Ack.

| +          continue;
| +        }
| +      if (strcmp (argv[i], env_prepend_opt) == 0)

This requires --lt-env-prepend foo=bar, rather than allowing
--lt-env-prepend=foo=bar; is it worth allowing both syntax forms for
consistency with GNU coding standards?  Actually, since the wrapper has
such a limited usage, I'm probably okay with the single form.

Added. (Not yet tested, but coded).

| +      /* otherwise ... */
| +      newargz[++newargc] = xstrdup (argv[i]);

Shouldn't you handle "--" as the end of wrapper options, in case the user
really wants to pass --lt-env-* as the first option to the wrapped executable?

Done.

| -        cat <<EOF
| -  execv ("$lt_newargv0", newargz);
| +        cat <<"EOF"
| +  execv (newargz[0], newargz);

I would rather see argv[0] in the wrapped executable as the original name
of the wrapper script, rather than newargz[0].  That way, messages printed
in the wrapped executable have the simpler name of the cwrapper (ie. it's
much nicer, when invoking 'm4', to see 'm4: error' than
'/path/to/lt-m4.exe: error', without having to know that 'm4' is just a
cwrapper).

Actually, you've backed into a bug: when computing newargv[0], if you trace the code, you'll see that the final component, the target exe's name, is (effectively)
   `basename $cwrapperexe .exe`.exe
If the actual target exe is lt-foo.exe, not foo.exe -- then the wrapper is already broken. THAT is a bug I need to fix.

Before, the script wrapper name was always cwrapper name (+_TMPwrapper or something). And libtool took care of ensuring that the script wrapper knew if the target exe had a funky lt- name. I need to use that same machinery here.

strcat can be inefficient if orig_value is long.  Why not cache the
lengths, then use strcpy into the correct offset?

Done. Still using strcat in main() because I don't want to uglify the code with 10 or so different length variables. However, maybe I can use lt_extend_str instead of straight-line code (downside: extra memory allocations, more stuff to free. Might be just as ugly...)

| +  strncpy (*name, arg, len-1);
| +  (*name)[len-1] = '\0';

coding style: s/len-1/len - 1/

Ack.

| +      char *new_value = lt_extend_str (getenv (name), value, 0);
| +      /* some systems can't cope with a ':'-terminated path #' */

What's up with the #' in the comment?

Syntax highlighter knows this file is a shell script, so it /doesn't/ know that /* is a comment marker. Therefore, it tries to highlight the "strings" between ' symbols. #' lets me balance those symbols on the line. But if it bothers you, I can remove it (or ensure balanced ' symbols by rewording).

--
Chuck




reply via email to

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