Re: shell portability: don't use `set --'

[Ralf Wildenhues]

* Gary V. Vaughan wrote on Wed, Dec 29, 2004 at 01:52:49AM CET:
> Ralf Wildenhues wrote:
> > * Gary V. Vaughan wrote on Fri, Dec 24, 2004 at 03:28:34PM CET:
> >>Ralf Wildenhues wrote:
> >>>If so, OK to apply to HEAD and branch-2-0?
> >>>
> >>>   * Makefile.am, bootstrap, clcommit.m4sh, libtoolize.m4sh,
> >>>   config/ltmain.m4sh: Replace `set --' with `set x [...]; shift'
> >>>   for portability.
> >>
> >>Actually, it is probably my fault for having let the -- idiom creep into
> >>libtool.  Up until 1.5 `set dummy ...' was used consistently throughout.
> >>
> >>I think we should continue in the same vein.  So, yes, but using dummy
> >>instead of x.  While we're at it, adding something akin to the following
> >>to sh.test should stop us (me!) accidentally doing it again:
> >
> > Thanks.  I applied the following, slightly simpler patch (including your
> > test) to HEAD and branch-2-0.
> 
> Hey, good call: Using shift is much cleaner.

Actually, my former patch had a bug somewhere there, and I figured it
was easier to redo like that than to go and look for the spot.  :-)

> That makes me think we should tighten the test to also error out if the
> shift is missing.  That would mean always having `; shift' on the same
> line as the `set dummy ...',

No, I think that's very ugly (to do in general).  And unnecessary..

>                              but might save us a slip up later.

Yes, maybe.

> If nobody beats me to it, I'll submit a patch when I get back from my
> folks in Devon later this week.

How 'bout this?  Turned out to be a little work cleaning up the
remaining failures of the new test..
(Not so sure if I like the results better; bit of a quick hack).

Regards,
Ralf

        * tests/sh.test: After `set dummy [...]', check for `shift'
        within the same and following line.
        * config/ltmain.m4sh (func_enable_tag, func_mode_install,
        func_mode_link): Sprinkle `shift's all over to confirm to this.

Index: tests/sh.test
===================================================================
RCS file: /cvsroot/libtool/libtool/tests/sh.test,v
retrieving revision 1.21
diff -u -r1.21 sh.test
--- tests/sh.test       28 Dec 2004 13:50:23 -0000      1.21
+++ tests/sh.test       29 Dec 2004 16:08:05 -0000
@@ -81,4 +81,14 @@
   status=$EXIT_FAILURE
 fi
 
+# Check for using shift after set dummy (same or following line).
+for s in $scripts
+do
+  if $SED -n '/set[    ][      
]*dummy/{/set.*dummy.*;.*shift/d;N;/set.*dummy.*\n.*shift/D;p;}' "$s" |
+     $EGREP .; then
+    echo "use \`shift' after \`set dummy' in $s"
+    status=$EXIT_FAILURE
+  fi
+done
+
 exit $status
Index: config/ltmain.m4sh
===================================================================
RCS file: /cvsroot/libtool/libtool/config/ltmain.m4sh,v
retrieving revision 1.39
diff -u -r1.39 ltmain.m4sh
--- config/ltmain.m4sh  28 Dec 2004 13:50:23 -0000      1.39
+++ config/ltmain.m4sh  29 Dec 2004 16:08:05 -0000
@@ -1780,8 +1780,8 @@
       destname="$func_basename_result"
 
       # Not a directory, so check to see that there is only one file specified.
-      set dummy $files
-      test "$#" -gt 2 && \
+      set dummy $files; shift
+      test "$#" -gt 1 && \
        func_fatal_help "\`$dest' is not a directory"
     fi
     case $destdir in
@@ -1872,10 +1872,9 @@
        fi
 
        # See the names of the shared library.
-       set dummy $library_names
-       if test -n "$2"; then
-         realname="$2"
-         shift
+       set dummy $library_names; shift
+       if test -n "$1"; then
+         realname="$1"
          shift
 
          srcname="$realname"
@@ -3285,8 +3284,8 @@
              valid_a_lib=no
              case $deplibs_check_method in
                match_pattern*)
-                 set dummy $deplibs_check_method
-                 match_pattern_regex=`expr "$deplibs_check_method" : "$2 
\(.*\)"`
+                 set dummy $deplibs_check_method; shift
+                 match_pattern_regex=`expr "$deplibs_check_method" : "$1 
\(.*\)"`
                  if eval $ECHO \"X$deplib\" 2>/dev/null | $Xsed -e 10q \
                    | $EGREP "$match_pattern_regex" > /dev/null; then
                    valid_a_lib=yes
@@ -3666,8 +3665,9 @@
          if test -n "$old_archive_from_expsyms_cmds"; then
            # figure out the soname
            set dummy $library_names
-           realname="$2"
-           shift; shift
+           shift
+           realname="$1"
+           shift
            libname=`eval \\$ECHO \"$libname_spec\"`
            # use dlname if we got it. it's perfectly good, no?
            if test -n "$dlname"; then
@@ -4180,10 +4180,11 @@
        func_warning "\`-dlopen self' is ignored for libtool libraries"
 
       set dummy $rpath
-      test "$#" -gt 2 && \
+      shift
+      test "$#" -gt 1 && \
        func_warning "ignoring multiple \`-rpath's for a libtool library"
 
-      install_libdir="$2"
+      install_libdir="$1"
 
       oldlibs=
       if test -z "$rpath"; then
@@ -4207,9 +4208,10 @@
        # Parse the version information argument.
        save_ifs="$IFS"; IFS=':'
        set dummy $vinfo 0 0 0
+       shift
        IFS="$save_ifs"
 
-       test -n "$8" && \
+       test -n "$7" && \
          func_fatal_help "too many parameters to \`-version-info'"
 
        # convert absolute version numbers to libtool ages
@@ -4218,9 +4220,9 @@
 
        case $vinfo_number in
        yes)
-         number_major="$2"
-         number_minor="$3"
-         number_revision="$4"
+         number_major="$1"
+         number_minor="$2"
+         number_revision="$3"
          #
          # There are really only two kinds -- those that
          # use the current revision as the major version
@@ -4247,9 +4249,9 @@
          esac
          ;;
        no)
-         current="$2"
-         revision="$3"
-         age="$4"
+         current="$1"
+         revision="$2"
+         age="$3"
          ;;
        esac
 
@@ -4572,8 +4574,8 @@
                if test -n "$i" ; then
                  libname=`eval \\$ECHO \"$libname_spec\"`
                  deplib_matches=`eval \\$ECHO \"$library_names_spec\"`
-                 set dummy $deplib_matches
-                 deplib_match=$2
+                 set dummy $deplib_matches; shift
+                 deplib_match=$1
                  if test `expr "$ldd_output" : ".*$deplib_match"` -ne 0 ; then
                    newdeplibs="$newdeplibs $i"
                  else
@@ -4614,8 +4616,8 @@
                  if test -n "$i" ; then
                    libname=`eval \\$ECHO \"$libname_spec\"`
                    deplib_matches=`eval \\$ECHO \"$library_names_spec\"`
-                   set dummy $deplib_matches
-                   deplib_match=$2
+                   set dummy $deplib_matches; shift
+                   deplib_match=$1
                    if test `expr "$ldd_output" : ".*$deplib_match"` -ne 0 ; 
then
                      newdeplibs="$newdeplibs $i"
                    else
@@ -4644,8 +4646,8 @@
          fi
          ;;
        file_magic*)
-         set dummy $deplibs_check_method
-         file_magic_regex=`expr "$deplibs_check_method" : "$2 \(.*\)"`
+         set dummy $deplibs_check_method; shift
+         file_magic_regex=`expr "$deplibs_check_method" : "$1 \(.*\)"`
          for a_deplib in $deplibs; do
            name="`expr $a_deplib : '-l\(.*\)'`"
            # If $name is empty we are operating on a -L argument.
@@ -4713,8 +4715,8 @@
          done # Gone through all deplibs.
          ;;
        match_pattern*)
-         set dummy $deplibs_check_method
-         match_pattern_regex=`expr "$deplibs_check_method" : "$2 \(.*\)"`
+         set dummy $deplibs_check_method; shift
+         match_pattern_regex=`expr "$deplibs_check_method" : "$1 \(.*\)"`
          for a_deplib in $deplibs; do
            name="`expr $a_deplib : '-l\(.*\)'`"
            # If $name is empty we are operating on a -L argument.
@@ -4923,8 +4925,9 @@
        eval shared_ext=\"$shrext_cmds\"
        eval library_names=\"$library_names_spec\"
        set dummy $library_names
-       realname="$2"
-       shift; shift
+       shift
+       realname="$1"
+       shift
 
        if test -n "$soname_spec"; then
          eval soname=\"$soname_spec\"