On 2012-09-22 05:31, Gary V. Vaughan wrote:
> Hi Peter,
>
> On 19 Sep 2012, at 21:50, Peter Rosin <p...@lysator.liu.se> wrote:
>
>> On 2012-09-19 16:20, Gary V. Vaughan wrote:
>>> Hi Peter,
>>>
>>> My bad, I'm embarrassed to say. I started to write a script to make the
>>> appropriate changes, but ended up doing it manually rather than adding
>>> more and more corner cases to the throwaway script... a poor choice in
>>> hindsight :-(
>>
>> It's easy to be wise after the fact... If you do redo it, may I suggest
>> breaking up the patch in smaller pieces for bisectability?
>
> I've pushed a temporary branch called gary/redo-test-operand-order to
> savannah with seven changesets that reverts the manual version of the
> buggy original and redoes it with a painstaking awk script (also checked
> in, for the morbidly curious).
>
> I'm on the fence about committing in smaller pieces... the policy for
> libtool has always been to make sure the testsuite passes (at least
> on the committer's machine) for every changeset, so that bisecting
> using one of the test cases doesn't fail unexpectedly on another
> commit that was intentionally pushed with know failures. On the other
> hand, the original was a monster, so I can see the benefits of splitting
> it up a bit too.
Why not commit the sc_prohibit_test_const_follows_var improvement
last, after fixing all violations?
BTW, the revert is also a monster, and I think the current split
is too coarse. It felt better with an autogenerated patch, but
that feeling disappeared when I reviewed the results, see below
for details. You still have hundreds of changes to fundamentally
different parts of the code in a single commit. It's impossible
(well, at least annoyingly time consuming) to find the offending
change if such a commit is found to cause a regression.
>>> I think it will be safer to revert the broken patch, and then
>>> write a script to reapply those changes automatically as I
>>> should have done originally, and only then to merge the result
>>> back to head. If you hold off for a few days, I'll do that as
>>> penance for my sins when I get back to my computer.
>>
>> I'm not sure a revert of such a big patch is possible after
>> 50-odd more patches? But I was thinking revert too after
>> reading it for a while... Who knows what else hides in
>> there?
>
> Well, I wrote and applied the script, diffed the results, and
> the testsuite has no regressions for me. I get a weird failure
> in distcheck which tries to run distclean in _build/tests/cdemo
> aftec removing _build/tests/cdemo/Makefile, that I haven't had
> time to check against current master to see if it is a new regression
> caused by my patch.
>
>>> But please hold on to your test case to verify that I've made
>>> a better job of things on the do over.
>>
>> That's easy, on Cygwin:
>> make check-local TESTSUITEFLAGS="-k Runpath"
>> is supposed to PASS (not FAIL) and
>> make check-local TESTSUITEFLAGS="-k relpaths"
>> is supposed to XFAIL (not XPASS)
>>
>> (mentioning it here so that I don't forget myself)
>
> Cool. When you have time, please let me know whether the temporary
> branch I made works properly for you. I'll do some more regression
> testing, and if all goes well, I'll transplant the branch onto
> master.
No issues in the testsuite here that I notice. Not trusting that
however...
> My awk script also matches your changes in these hunks, so I'm
> moderately confident that it will have caught any other lurkers too.
...and diffing between master (-) and redo-test-operand-order (+) got
me these among a bunch of booooring stuff. But I wonder what I missed
this time?
diff --git a/m4/argz.m4 b/m4/argz.m4
index ad1743e..09568ad 100644
--- a/m4/argz.m4
+++ b/m4/argz.m4
@@ -55,11 +55,11 @@ AS_IF([test -z "$ARGZ_H"],
lt_os_major=${2-0}
lt_os_minor=${3-0}
lt_os_micro=${4-0}
- if test 1 -le "$lt_os_major" \
+ if test 1 -lt "$lt_os_major" \
|| { test 1 -eq "$lt_os_major" \
- && { test 5 -le "$lt_os_minor" \
+ && { test 5 -lt "$lt_os_minor" \
|| { test 5 -eq "$lt_os_minor" \
- && test 24 -le "$lt_os_micro"; }; }; }; then
+ && test 24 -lt "$lt_os_micro"; }; }; }; then
lt_cv_sys_argz_works=yes
fi
fi
diff --git a/m4/libtool.m4 b/m4/libtool.m4
index 4413a6d..8ec9beb 100644
--- a/m4/libtool.m4
+++ b/m4/libtool.m4
@@ -4982,7 +4982,7 @@ _LT_EOF
# need to do runtime linking.
case $host_os in aix4.[[23]]|aix4.[[23]].*|aix[[5-9]]*)
for ld_flag in $LDFLAGS; do
- if (test x-brtl = "x$ld_flag" || test x-Wl,-brtl = "x$ld_flag"); then
+ if (test -brtl = "$ld_flag" || test -Wl,-brtl = "$ld_flag"); then
aix_use_runtimelinking=yes
break
fi
@@ -7730,7 +7730,7 @@ for lt_ac_sed in $lt_ac_sed_list /usr/xpg4/bin/sed; do
$lt_ac_sed -e 's/a$//' < conftest.nl >conftest.out || break
cmp -s conftest.out conftest.nl || break
# 10000 chars as input seems more than enough
- test 10 -le "$lt_ac_count" && break
+ test 10 -lt "$lt_ac_count" && break
lt_ac_count=`expr $lt_ac_count + 1`
if test "$lt_ac_count" -gt "$lt_ac_max"; then
lt_ac_max=$lt_ac_count
Looks like at least these lines need improvement:
# reverse -lt and -lte operand order
line = gensub (lhsx "-l(te?)" rhsx, "\\1\\4 -g\\3 \"\\2\"\\5", "g",
line);
line = gensub (lhs "-l(te?)" rhs, "\\1\\4 -g\\3 \"\\2\"\\5", "g",
line);
# reverse -gt and -gte operand order
line = gensub (lhsx "-g(te?)" rhsx, "\\1\\4 -l\\3 \"\\2\"\\5", "g",
line);
line = gensub (lhs "-g(te?)" rhs, "\\1\\4 -l\\3 \"\\2\"\\5", "g",
line);
What's this gte/lte thing anyway? Isn't it supposed to be ge/le?
That doesn't explain the dangerous looking -brtl change.
> If not, I will branch before 962aa91, run the script, and then apply
> the rest of master to that branch one patch at a time until I arrive
> at a diff that I can apply to master itself, rather than using revert
> as I did on the current temporary branch.
I have to say, given all these difficulties, is it really worth it?
Besides, I think
test $X -eq 5
is much more readable than
test 5 -eq $X
simply because it matches the way I'm thinking, i.e. if X equals 5
then do something. I never think the other way around, and that is
more of a burden to readability than to scan past the odd x if that's
needed to stop the possibility of special chars. But maybe that's
just me?
Cheers,
Peter
