-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
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.
|
| I'm sorta thinking I should rename the func_to_native* functions
s/native/host/ ?
Yes, that would be appropriate. I'd like to see that, and other cleanups
below, before approving. I haven't tested yet, but the concepts look sane
by inspection.
| +func_to_native_path ()
| +{
| + func_to_native_path_result="$1"
| + if test -n "$1" ; then
| + case $host in
| + *mingw* )
| + lt_sed_naive_backslashify='s|\\\\*|\\|g;s|/|\\|g;s|\\|\\\\|g'
| + case $build in
| + *mingw* ) # actually, msys
| + # awkward: cmd appends spaces to result
| + lt_sed_strip_trailing_spaces="s/[ ]*\$//"
| + func_to_native_path_tmp1=`( cmd //c echo "$1" | $SED -e
"$lt_sed_strip_trailing_spaces" ) 2>/dev/null || echo ""`
| + func_to_native_path_result=`echo
"$func_to_native_path_tmp1" | $SED -e "$lt_sed_naive_backslashify"`
Some pretty long lines; is it worth trying to wrap at 80 columns?
| + ;;
| + *cygwin* )
| + func_to_native_path_tmp1=`cygpath -w "$1"`
| + func_to_native_path_result=`echo
"$func_to_native_path_tmp1" | $SED -e "$lt_sed_naive_backslashify"`
| + ;;
| + esac
| + if test -z "$func_to_native_path_result" ; then
| + func_error "Could not determine native path corresponding to"
| + func_error " '$1'"
| + func_error "Perhaps it doesn't exist."
| + func_error "Continuing, but running uninstalled executables
may not work."
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...
| +func_to_native_pathlist ()
| +{
| + func_to_native_pathlist_result="$1"
| + if test -n "$1" ; then
| + case $host in
| + *mingw* )
| + lt_sed_naive_backslashify='s|\\\\*|\\|g;s|/|\\|g;s|\\|\\\\|g'
| + case $build in
| + *mingw* | *cygwin* )
| + # remove leading and trailing ':' from $1. msys behavior is
| + # inconsistent here, and cygpath turns them into into '.;'
and ';.'
| + 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.
| +
| +static const char *dumpscript_opt = "--lt-dump-script";
| +static const char *env_set_opt = "--lt-env-set";
| + /* argument is putenv-style "foo=bar", value of foo is set to bar */
| +static const char *env_prepend_opt = "--lt-env-prepend";
| + /* argument is putenv-style "foo=bar", new value of foo is bar${foo} */
| +static const char *env_append_opt = "--lt-env-append";
| + /* argument is putenv-style "foo=bar", new value of foo is ${foo}bar */
It's a bit of a shame that we can't rely on getopt_long and get option
abbreviations; oh well.
| - 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.
| + target_name = (char *) xstrdup (base_name (actual_cwrapper_path));
Why the cast? Shouldn't xstrdup output char* already? 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.
| + if (i+1 < argc)
Coding style: s/i+1/i + 1/
| + {
| + 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.
| + 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.
| + /* 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?
| - 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).
| +lt_extend_str (const char *orig_value, const char *add, int to_end)
| +{
| + char *new_value;
| + if (orig_value && *orig_value)
| + {
| + int len = strlen (add) + strlen (orig_value) + 1;
| + new_value = XMALLOC (char, len);
| + if (to_end)
| + {
| + strcpy (new_value, orig_value);
| + strcat (new_value, add);
strcat can be inefficient if orig_value is long. Why not cache the
lengths, then use strcpy into the correct offset?
| + strncpy (*name, arg, len-1);
| + (*name)[len-1] = '\0';
coding style: s/len-1/len - 1/
| + 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?
- --
Don't work too hard, make some time for fun as well!
Eric Blake [EMAIL PROTECTED]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkggWzYACgkQ84KuGfSFAYCnxQCgtjmEmZHH1OtgxLT3ACCYwW+6
iBMAoMpRj2Hqpi+SZhjoTANv/4I49Gy/
=t9g7
-----END PGP SIGNATURE-----
