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

[Bernhard Voelker]

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.

Have a nice day,
Berny

[1] http://kernel.org/doc/Documentation/email-clients.txt


>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

* tests/misc/stty: Add iutf8 to the list of REV_* options.
That option has been implemented in commit v5.2.1-193-g733e79e.
---
 tests/misc/stty |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/misc/stty b/tests/misc/stty
index 1b082c2..cfc70c1 100755
--- a/tests/misc/stty
+++ b/tests/misc/stty
@@ -29,13 +29,13 @@ REV_parenb=1 REV_parodd=1 REV_hupcl=1 REV_hup=1 
REV_cstopb=1 REV_cread=1
 REV_clocal=1 REV_crtscts=1 REV_ignbrk=1 REV_brkint=1 REV_ignpar=1
 REV_parmrk=1 REV_inpck=1 REV_istrip=1 REV_inlcr=1 REV_igncr=1 REV_icrnl=1
 REV_ixon=1 REV_ixoff=1 REV_tandem=1 REV_iuclc=1 REV_ixany=1 REV_imaxbel=1
-REV_opost=1 REV_olcuc=1 REV_ocrnl=1 REV_onlcr=1 REV_onocr=1 REV_onlret=1
-REV_ofill=1 REV_ofdel=1 REV_isig=1 REV_icanon=1 REV_iexten=1 REV_echo=1
-REV_echoe=1 REV_crterase=1 REV_echok=1 REV_echonl=1 REV_noflsh=1
-REV_xcase=1 REV_tostop=1 REV_echoprt=1 REV_prterase=1 REV_echoctl=1
-REV_ctlecho=1 REV_echoke=1 REV_crtkill=1 REV_evenp=1 REV_parity=1
-REV_oddp=1 REV_nl=1 REV_cooked=1 REV_raw=1 REV_pass8=1 REV_litout=1
-REV_cbreak=1 REV_decctlq=1 REV_tabs=1 REV_lcase=1 REV_LCASE=1
+REV_iutf8=1 REV_opost=1 REV_olcuc=1 REV_ocrnl=1 REV_onlcr=1 REV_onocr=1
+REV_onlret=1 REV_ofill=1 REV_ofdel=1 REV_isig=1 REV_icanon=1 REV_iexten=1
+REV_echo=1 REV_echoe=1 REV_crterase=1 REV_echok=1 REV_echonl=1
+REV_noflsh=1 REV_xcase=1 REV_tostop=1 REV_echoprt=1 REV_prterase=1
+REV_echoctl=1 REV_ctlecho=1 REV_echoke=1 REV_crtkill=1 REV_evenp=1
+REV_parity=1 REV_oddp=1 REV_nl=1 REV_cooked=1 REV_raw=1 REV_pass8=1
+REV_litout=1 REV_cbreak=1 REV_decctlq=1 REV_tabs=1 REV_lcase=1 REV_LCASE=1


 saved_state=.saved-state
-- 
1.7.7


>From 300b08919fca2c2e65122d76f9ed45b5317845b5 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <address@hidden>
Date: Tue, 10 Apr 2012 10:10:50 +0200
Subject: [PATCH 2/2] 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.
---
 tests/Makefile.am     |    1 +
 tests/init.cfg        |   27 +++++++++++++++++++++
 tests/misc/stty       |   42 ++------------------------------
 tests/misc/stty-pairs |   63 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 39 deletions(-)
 create mode 100755 tests/misc/stty-pairs

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 011051a..86afbc2 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -274,6 +274,7 @@ TESTS =                                             \
   misc/stdbuf                                  \
   misc/stty                                    \
   misc/stty-invalid                            \
+  misc/stty-pairs                              \
   misc/stty-row-col                            \
   misc/su-fail                                 \
   misc/sum                                     \
diff --git a/tests/init.cfg b/tests/init.cfg
index 7bcd512..08645d1 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -241,6 +241,33 @@ 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' ' ')
+  # 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)  \
+    || framework_failure_ "too few reversible settings"
+}
+
+# Test if $1 is among the reversible stty options ($REV).
+stty_reversible_matches()
+{
+  case "$REV" in
+    "")
+      framework_failure_ "stty_reversible_settings() not called?";;
+    *" $1 "*)
+      return 0;;
+    *)
+      return 1;;
+  esac
+}
+
 skip_if_()
 {
   case $1 in
diff --git a/tests/misc/stty b/tests/misc/stty
index cfc70c1..10aaa0a 100755
--- a/tests/misc/stty
+++ b/tests/misc/stty
@@ -23,20 +23,8 @@ print_ver_ stty
 require_controlling_input_terminal_
 trap '' TTOU # Ignore SIGTTOU

-# The following list of reversible options was generated with
-# grep -w REV stty.c|sed -n '/^  {"/{s//REV_/;s/".*/=1/;p;}'|fmt
-REV_parenb=1 REV_parodd=1 REV_hupcl=1 REV_hup=1 REV_cstopb=1 REV_cread=1
-REV_clocal=1 REV_crtscts=1 REV_ignbrk=1 REV_brkint=1 REV_ignpar=1
-REV_parmrk=1 REV_inpck=1 REV_istrip=1 REV_inlcr=1 REV_igncr=1 REV_icrnl=1
-REV_ixon=1 REV_ixoff=1 REV_tandem=1 REV_iuclc=1 REV_ixany=1 REV_imaxbel=1
-REV_iutf8=1 REV_opost=1 REV_olcuc=1 REV_ocrnl=1 REV_onlcr=1 REV_onocr=1
-REV_onlret=1 REV_ofill=1 REV_ofdel=1 REV_isig=1 REV_icanon=1 REV_iexten=1
-REV_echo=1 REV_echoe=1 REV_crterase=1 REV_echok=1 REV_echonl=1
-REV_noflsh=1 REV_xcase=1 REV_tostop=1 REV_echoprt=1 REV_prterase=1
-REV_echoctl=1 REV_ctlecho=1 REV_echoke=1 REV_crtkill=1 REV_evenp=1
-REV_parity=1 REV_oddp=1 REV_nl=1 REV_cooked=1 REV_raw=1 REV_pass8=1
-REV_litout=1 REV_cbreak=1 REV_decctlq=1 REV_tabs=1 REV_lcase=1 REV_LCASE=1
-
+# Get the reversible settings from stty.c.
+stty_reversible_settings

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

   # Likewise, 'stty -cread' would fail, so skip that, too.
   test $opt = cread && continue
-  eval rev=\$REV_$opt
-  if test -n "$rev"; then
+  if stty_reversible_matches "$opt" ; then
     stty -$opt || { fail=1; echo -$opt; }
   fi
 done

-if test -n "$RUN_LONG_TESTS"; then
-  # Take them in pairs.
-  for opt1 in $options; do
-    echo .|tr -d '\n'
-    for opt2 in $options; do
-
-      stty $opt1 $opt2 || fail=1
-
-      eval rev1=\$REV_$opt1
-      eval rev2=\$REV_$opt2
-      if test -n "$rev1"; then
-        stty -$opt1 $opt2 || fail=1
-      fi
-      if test -n "$rev2"; then
-        stty $opt1 -$opt2 || fail=1
-      fi
-      if test "$rev1$rev2" = 11; then
-        stty -$opt1 -$opt2 || fail=1
-      fi
-    done
-  done
-fi
-
 stty $(cat $saved_state)

 Exit $fail
diff --git a/tests/misc/stty-pairs b/tests/misc/stty-pairs
new file mode 100755
index 0000000..81ee46b
--- /dev/null
+++ b/tests/misc/stty-pairs
@@ -0,0 +1,63 @@
+#!/bin/sh
+# Make sure stty can parse most of its options - in pairs [expensive].
+
+# Copyright (C) 1998-2012 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ stty
+
+expensive_
+
+# Make sure there's a tty on stdin.
+require_controlling_input_terminal_
+trap '' TTOU # Ignore SIGTTOU
+
+# Get the reversible settings from stty.c.
+stty_reversible_settings
+
+saved_state=.saved-state
+stty --save > $saved_state || fail=1
+stty $(cat $saved_state) || fail=1
+
+# Build a list of all boolean options stty accepts on this system.
+# Don't depend on terminal width.  Put each option on its own line,
+# remove all non-boolean ones, remove 'parenb' and 'cread' explicitly,
+# then remove any leading hyphens.
+sed_del='/^speed/d;/^rows/d;/^columns/d;/ = /d;s/parenb//;s/cread//'
+options=$(stty -a | tr -s ';' '\n' | sed "s/^ //;$sed_del;s/-//g")
+
+# Take them in pairs, with and without the leading '-'.
+for opt1 in $options; do
+  for opt2 in $options; do
+
+    stty $opt1 $opt2 || fail=1
+
+    if stty_reversible_matches "$opt1" ; then
+      stty -$opt1 $opt2 || fail=1
+    fi
+    if stty_reversible_matches "$opt2" ; then
+      stty $opt1 -$opt2 || fail=1
+    fi
+    if stty_reversible_matches "$opt1" \
+    && stty_reversible_matches "$opt2" ; then
+      stty -$opt1 -$opt2 || fail=1
+    fi
+  done
+done
+
+stty $(cat $saved_state)
+
+Exit $fail
-- 
1.7.7