Re: ylwrap proposition

[Ralf Wildenhues]

Hello Sergey,

* Sergey Poznyakoff wrote on Fri, Jun 22, 2007 at 01:15:32PM CEST:
> 
> Some (rather long) time ago, I proposed an extension to ylwrap that
> would enable it to provide a functionality similar to that
> of `bison -p prefix', i.e. replacing `yy' prefix in idenifiers with
> the given string. (The original proposition is available here:
> http://lists.gnu.org/archive/html/bug-automake/2001-06/msg00021.html)
> 
> Tom seemed to agree with this. However, the patch never made it to the
> distribution. So, here is the similar patch, this time against the
> CVS automake.

Thanks for the patch.  I'm not very experienced with yacc, so some of
the following questions may seem stupid:

Can s/yy/.../ change anything unintended?  E.g., do we need to ensure it
only changes words beginning with yy?  Can it omit anything that needs
changed (what about `YY...')?  What about the header file, doesn't that
need to be changed as well?  Does the renaming work for extensions such
as bison's for C++ parsers?  ylwrap is used for both lex and yacc:
should we disable the functionality for lex (as it may produce garbage
there?), and should it be made clear in the help output that -p is only
for the yacc part?

This patch needs the following things to be complete.  If you are
willing to help with any of them, that'd be great.  Otherwise it may
be a little while until I get to it:

- update scriptversion and copyright year in the ylwrap script.
- add test(s) to ensure the new functionality works as expected.
  tests/yacc8.test can serve as a starting point for a new test.
- write ChangeLog entry, adjust NEWS, THANKS, doc/automake.texi:
  mention new option, change recommendation about multiple parsers
  (probably the list of #defines should not be removed just yet,
  it may be interesting if only for historical purposes).
- test with 2-3 yacc implementations (I have access to some systems).
  Ensure none of the other ylwrap-related tests fail.
- put the patch in HEAD: this is a new feature so not eligible for
  branch-1-10.

Some nits:

> Index: lib/ylwrap
> --- automake/lib/ylwrap       2007-06-22 11:57:46.000000000 +0300
> +++ automake.orig/lib/ylwrap  2007-06-22 13:05:20.000000000 +0300
> @@ -41,9 +41,13 @@ case "$1" in
>      basedir=$2
>      shift 2
>      ;;
> +  --p|--pr|--pre|--pref|--prefi|--prefix)

-p is not actually supported, but documented.

> +    YYREPL="s/yy/$2/g"
> +    shift 2
> +    ;;

> @@ -177,7 +186,8 @@ if test $ret -eq 0; then
>              -e 'y/abcdefghijklmnopqrstuvwxyz/ABCDEFGHIJKLMNOPQRSTUVWXYZ/'\
>              -e 's/[^ABCDEFGHIJKLMNOPQRSTUVWXYZ]/_/g'`
>  
> -      sed -e "/^#/!b" -e "s,$input_rx,," -e "s,$from,$2," \
> +      sed -e "$YYREPL"  \

The command
  sed -e ""

is actually not portable, it will fail with AIX 5.3 sed:
$ echo x | sed -e ""
| sed: 0602-429 No editing script was provided.
| Usage:  sed [-n] Script [File ...]
|         sed [-n] [-e Script] ... [-f Script_file] ... [File ...]

Let's just rewrite the whole thing like:

  sed -e "
    $YYREPL
    /^#/!b
    s,$input_rx,,
    s,$from,$2,
    s,$FROM,$TARGET," "$from" >"$target" || ret=$?

and really the variables used in the sed substitutions could use more
escaping (for example comma needs to be escaped).  This latter bug has
been present before your patch, of course.

> +          -e "/^#/!b" -e "s,$input_rx,," -e "s,$from,$2," \
>            -e "s,$FROM,$TARGET," "$from" >"$target" || ret=$?
>  
>        # Check whether header files must be updated.

Cheers,
Ralf