[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: shell portability: don't use `set --'
From: |
Ralf Wildenhues |
Subject: |
Re: shell portability: don't use `set --' |
Date: |
Thu, 30 Dec 2004 13:50:55 +0100 |
User-agent: |
Mutt/1.4.1i |
* 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\"