Re: [PATCH] refactor expensive code in misc/stty [was: amendments to backtick-removing series]

[Jim Meyering]

Bernhard Voelker wrote:
> CC'ing Sami as he sent another note privately.
>
> On 04/05/2012 10:01 AM, Jim Meyering wrote:
>
>> So iutf8 was added to stty.c (by me, 8 years ago), but not here,
>> so never exposed to these tests.  Good catch.
>
> Thanks.
>
>> What do you think about eval'ing the output of the command
>> just above, instead.  Then this won't happen again.
>
> I thought about that (but read on) ...
>
>
>> Or better, a function in init.cfg,
>>
>> stty_reversible_settings()
>> {
>>   local r
>>   r=$(perl -lne '/^ *{"(.*?)",.*\bREV\b/ and print $1' \
>>       $abs_top_srcdir/src/stty.c)
>>   # Ensure that there are at least 62, i.e., so we're alerted if
>>   # reformatting the source empties the list.
>>   test 62 -lt $(echo "$r"|wc -l) || framework_failure_
>>   echo "$r"
>> }
>
> a) s/lt/le/ ;-)

Yes, definitely.  Initially I had 60 there, but changed it
to match 62 (current total) at the last minute.
Moral: don't make last-minute changes.

>> Then, you could add this one-liner in each of those two files,
>> in place of the hard-coded list of REV_... variable settings:
>>
>>   # Set REV_$S=1 for each reversible stty setting, $S.
>>   eval $(stty_reversible_settings |sed 's/^/REV_/;s/$/=1/')
>>
>
> b) framework_failure_ in a subshell doesn't work.
> c) I don't like eval too much, it's harder to read.
>
> What about the following?

It's already an expensive test, but even so, adding four $(...) subshell
invocations in an inner loop that is run ~2-3000 times seems like it
would have non-negligible cost, especially on systems where subshells
are relatively expensive.

I guess I prefer the original eval-using code,
if you can find a way to make it work.

One caveat: I realized that the code on HEAD
is slightly vulnerable to unwarranted failure
because we use some $REV_... variables without
first ensuring they're set.  So if certain ones
happen to be set in the environment, they'll cause
this test to fail.

One way to avoid that is to initialize $REV_...=0
for each valid setting.

BTW, your patch below was corrupted because something
along the way converted non-leading TABs to spaces.

> P.S. I could've even overloaded the function in init.cfg
> to get the list when no arg is given, and otherwise test if
> the arg is in the internal list, but I think this is safer.
>
> Have a nice day,
> Berny
>
>
>>From b8e5e7e9f25d21284daffeca65634840dfb1d0c1 Mon Sep 17 00:00:00 2001
> From: Bernhard Voelker <address@hidden>
> Date: Thu, 5 Apr 2012 12:05:46 +0200
> Subject: [PATCH] tests: factor out expensive "pairs" code of misc/stty
>
> * tests/Makefile.am: Add misc/stty-pairs.
> * tests/init.cfg (stty_reversible_settings): Add new function to get
> the list of reversible options from stty.c.
> * tests/init.cfg (stty_reversible_matches): Add new function to test
> wether a given option is in the list of reversible options.
> * tests/misc/stty: Factor out expensive "pairs" code into new test.
> Use new stty_reversible_settings and stty_reversible_matches instead
> of evaluating static REV_* variables.
> * tests/misc/stty-pairs: Added new test. Code added from misc/stty.
> Mark this as an expensive test. Skip 'parenb' and 'cread' options,
> as these tests are known to fail. Like in misc/stty, also use
> new stty_reversible_settings and stty_reversible_matches.