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

[Jim Meyering]

Bernhard Voelker wrote:

> On 04/05/2012 10:45 PM, Jim Meyering wrote:
>> Bernhard Voelker wrote:
>>> 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.
>
> ah yes, the subshells are silly. I just removed them.
> The time for this test is now down from 13s with the
> original master version to 8 seconds.
>
> I kept sticking to replace the eval-using code because - as
> the variable store is now used from 2 test files - I liked
> the idea that the "how the REVs are stored" is encapsulated
> centrally in the 2 new shell functions in init.cfg.
>
>> BTW, your patch below was corrupted because something
>> along the way converted non-leading TABs to spaces.
>
> Argh, it wasn't Thunderbird this time. I copied the patch from
> an xterm, and during this, the TABs in Makefile.am have been
> replaced (see also [1]). xclip is doing better.

>>From f71ed9ae1a9699e000a98c26dae4db96a628d657 Mon Sep 17 00:00:00 2001
> From: Bernhard Voelker <address@hidden>
> Date: Thu, 5 Apr 2012 08:01:43 +0200
> Subject: [PATCH 1/2] tests: add iutf8 option to misc/stty
...

Oops.  This slipped off my radar.
Sorry about that.

The comment below wasn't quite right and I'd rather not
use REV as a global variable.  Too short.
Similarly, I've renamed the function names slightly
and added a trailing underscore to each.  The trailing
underscore is probably not absolutely necessary (and we haven't
been very consistent about that), but it's a good practice.

Here are proposed incremental changes:

diff --git a/tests/init.cfg b/tests/init.cfg
index 08645d1..2e43c16 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -241,26 +241,26 @@ rwx_to_mode_()
   echo "=$u$g$o"
 }

-# Return the reversible settings from stty.c.
-# Fill the variable REV which is needed by stty_reversible_matches.
-stty_reversible_settings()
-{
-  # Pad start with one blank for the first option
-  # to match in stty_reversible_matches.
-  REV=" "$(perl -lne '/^ *{"(.*?)",.*\bREV\b/ and print $1' \
-      $abs_top_srcdir/src/stty.c | tr '\n' ' ')
+# Set the global variable stty_reversible_ to a space-separated list of the
+# reversible settings from stty.c.  stty_reversible_ also starts and ends
+# with a space.
+stty_reversible_init_()
+{
+  # Pad start with one space for the first option to match in query function.
+  stty_reversible_=' '$(perl -lne '/^ *{"(.*?)",.*\bREV\b/ and print $1' \
+    $abs_top_srcdir/src/stty.c | tr '\n' ' ')
   # Ensure that there are at least 62, i.e., so we're alerted if
   # reformatting the source empties the list.
-  test 62 -le $(echo "$REV"|wc -w)  \
+  test 62 -le $(echo "$stty_reversible_"|wc -w)  \
     || framework_failure_ "too few reversible settings"
 }

-# Test if $1 is among the reversible stty options ($REV).
-stty_reversible_matches()
+# Test whether $1 is one of stty's reversible options.
+stty_reversible_query_()
 {
-  case "$REV" in
-    "")
-      framework_failure_ "stty_reversible_settings() not called?";;
+  case $stty_reversible_ in
+    '')
+      framework_failure_ "stty_reversible_init_() not called?";;
     *" $1 "*)
       return 0;;
     *)
diff --git a/tests/misc/stty b/tests/misc/stty
index 10aaa0a..ae65656 100755
--- a/tests/misc/stty
+++ b/tests/misc/stty
@@ -24,7 +24,7 @@ require_controlling_input_terminal_
 trap '' TTOU # Ignore SIGTTOU

 # Get the reversible settings from stty.c.
-stty_reversible_settings
+stty_reversible_init_

 saved_state=.saved-state
 stty --save > $saved_state || fail=1
@@ -58,7 +58,7 @@ for opt in $options; do

   # Likewise, 'stty -cread' would fail, so skip that, too.
   test $opt = cread && continue
-  if stty_reversible_matches "$opt" ; then
+  if stty_reversible_query_ "$opt" ; then
     stty -$opt || { fail=1; echo -$opt; }
   fi
 done
diff --git a/tests/misc/stty-pairs b/tests/misc/stty-pairs
index 81ee46b..e59da04 100755
--- a/tests/misc/stty-pairs
+++ b/tests/misc/stty-pairs
@@ -26,7 +26,7 @@ require_controlling_input_terminal_
 trap '' TTOU # Ignore SIGTTOU

 # Get the reversible settings from stty.c.
-stty_reversible_settings
+stty_reversible_init_

 saved_state=.saved-state
 stty --save > $saved_state || fail=1
@@ -45,14 +45,14 @@ for opt1 in $options; do

     stty $opt1 $opt2 || fail=1

-    if stty_reversible_matches "$opt1" ; then
+    if stty_reversible_query_ "$opt1" ; then
       stty -$opt1 $opt2 || fail=1
     fi
-    if stty_reversible_matches "$opt2" ; then
+    if stty_reversible_query_ "$opt2" ; then
       stty $opt1 -$opt2 || fail=1
     fi
-    if stty_reversible_matches "$opt1" \
-    && stty_reversible_matches "$opt2" ; then
+    if stty_reversible_query_ "$opt1" \
+        && stty_reversible_query_ "$opt2" ; then
       stty -$opt1 -$opt2 || fail=1
     fi
   done