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

[Bernhard Voelker]

On 04/19/2012 05:58 PM, Jim Meyering wrote:
>> >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.

no worries - I'm glad you liked my idea to use a shell's
case statement in a reverse way ... for the reverse options ;-)


> 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.

thank you, good idea.

> 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;;
>      *)

You removed the double quotes around the case variable.
For whatever it is good for, I'd have a better feeling if they were
there ... imagine something goes wrong during the perl processing.
Maybe that's only my paranoia ;-)

Apart from that, it looks good.

Have a nice day,
Berny