Re: [PATCH 3/3] ylwrap: rename header inclusion in generated parsers

[Stefano Lattarini]

Hi Akim.

On 07/13/2012 01:20 PM, Akim Demaille wrote:
> 
> Le 12 juil. 2012 à 18:22, Stefano Lattarini a écrit :
> 
>> On 07/12/2012 03:51 PM, Akim Demaille wrote:
>>> * lib/am/yacc.am (am__yacc_c2h): Shorten.
>>>
>> See below.
>>
>>> @@ -37,8 +37,7 @@ if %?MAINTAINER-MODE%
>>> @address@hidden = test -f $@ ||
>>> endif %?MAINTAINER-MODE%
>>> ## The 's/c$/h/' substitution *must* be the last one.
>>> -am__yacc_c2h = sed -e s/cc$$/hh/ -e s/cpp$$/hpp/ -e s/cxx$$/hxx/ \
>>> -              -e s/c++$$/h++/ -e s/c$$/h/
>>> +am__yacc_c2h = sed 
>>> 's/cc$$/hh/;s/cpp$$/hpp/;s/cxx$$/hxx/;s/c++$$/h++/;s/c$$/h/'
>>> endif %?FIRST%
>>>
>> What has this change to do with this patch?.  Seems only cosmetic munging to
>> me, not required by other changes, and should thus be done in a separate 
>> patch.
>> And before yo go writing such a follow-up patch, consider that I find the
>> pre-existing, slightly longer formulation clearer, so the commit message 
>> should
>> give some rationale about why the new formulation should be preferred.
> 
> Well running the test case, the command is displayed, and it is uselessly
> long, which make reading the compilation line harder.  If you don't think
> this is more readable, I really don't care dropping it.
>
Yes, drop it from this patch please.  If then you want to prepare a follow-up
patch with this change (and only this change) and the just-given rationale in
the commit messages, I'll take it.

>>> * lib/ylwrap (rename_sed): New.
>>> (main loop): Use it the rename the dependencies to other files.
>>>
>> Oops, diving straight into the details, no rationale.  No good.
> 
> Well, I believed the title to be clear enough, but ok.
> 
>> As an outsider, it is not clear to me why this change is useful, nor
>> how it fits in the bigger picture of Bison development.
> 
> It's not only Bison's future changes, it is that it is already
> wrong from all the other skeletons.
>
See?  Another thing I had got wrong, given my ignorance and the lack
of a proper explanation ;-)

>>  That should
>> be made clear in the commit message.  Extra points if you can also add
>> links to the relevant discussion/patches in the Bison lists.
> 
> Ok.
>
Thanks.

>>> ?GENERIC?%EXT%%DERIVED-EXT%:
>>> diff --git a/lib/ylwrap b/lib/ylwrap
>>> index 06b4706..0b6ae10 100755
>>> --- a/lib/ylwrap
>>> +++ b/lib/ylwrap
>>> @@ -113,6 +113,8 @@ fi
>>>
>>> # The list of file to rename: FROM TO...
>>> pairlist=
>>> +# A sed program to s/FROM/TO/g for all the FROM/TO.
>>> +rename_sed=
>>> while test "$#" -ne 0; do
>>>   if test "$1" = "--"; then
>>>     shift
>>> @@ -130,6 +132,7 @@ while test "$#" -ne 0; do
>>>   to=$1
>>>   shift
>>>   pairlist="$pairlist $from $to"
>>> +  rename_sed="${rename_sed}s,$from,$to,g;"
>>>
>> Hmmm... can '$from' contain sed metacharacters?  Certainly it can contain
>> dots; still, that wouldn't cause spurious failures, only possible (albeit
>> very unlikely) extra substitutions in "#include" lines; which I agree we
>> don't need worry about, due to their very high unlikeliness.  So the code
>> above is correct IMO, but it deserves some more comments, so that a future
>> reader won't have to repeat my chain of thoughts.
> 
> The code is exactly the same as what was there before, just moved
> elsewhere.
>
A good excuse to improve comments and explanations, no?

>  But I can add protections, sure.
>
Oops, I fear you misread my feedback; I agree that extra protections are
overkill, I'd just like you to add a brief comments (along my reasoning
above) to explain why this is the case.

> Done below.
>
Still, upon reading this new change, I think it is small and clear that
I'm OK with keeping it.  Thanks.

>> Also, a minor and unrelated nit (feel free to ignore this): I think that,
>> in the Autotools code base, 's|||' in preferred over 's,,,'  when the
>> regexp or the substitution can contain file names (Eric Blake hinted at
>> this in a message few days ago, but I forgot on which list).
> 
> OK.  So since I was just using the historical conventions, that
> used to be ',', that are used in ylwrap, I understand your
> request as a need to change all the other patterns, not just
> the one I moved.
>
Oops, no; that should be done in a follow-up patch if you are interested.
Sorry for not stating that explicitly.  Please revert this part of the
change.

>>
>>> done
>>>
>>> # The program to run.
>>> @@ -187,16 +190,14 @@ if test $ret -eq 0; then
>>>         realtarget="$target"
>>>         target="tmp-`echo $target | sed s/.*[\\/]//g`"
>>>       fi
>>> -      # Munge "#line" or "#" directives.
>>> -      # We don't want the resulting debug information to point at
>>> -      # an absolute srcdir.
>>> -      # We want to use the real output file name, not yy.lex.c for
>>> -      # instance.
>>> -      # We want the include guards to be adjusted too.
>>> +      # Munge "#line" or "#" directives.  We don't want the resulting
>>> +      # debug information to point at an absolute srcdir.  We want to
>>> +      # use the real output file name, not yy.lex.c for instance.  We
>>> +      # want the include guards to be adjusted too.
>>>
>> This tweaking might also have been part of later commit, but no big deal,
>> being just comments (and me preferring your new formulation better ;-).
>> Still, this change should be reported in the commit message:
>>
>>    * lib/ylwrap (rename_sed): New.
>>    (main loop): Use it the rename the dependencies to other files.
>>  + Improve few comments while we are at it.
> 
> I just wrapped it, probably by accident, but ok, I improved it.
>
>>>       FROM=`guard "$from"`
>>>       TARGET=`guard "$to"`
>>>
>>> -      sed -e "/^#/!b" -e "s,$input_rx,$input_sub_rx," -e "s,$from,$to," \
>>> +      sed -e "/^#/!b" -e "s,$input_rx,$input_sub_rx," -e "$rename_sed" \
>>>           -e "s,$FROM,$TARGET," "$from" >"$target" || ret=$?
>>>
>>>       # Check whether header files must be updated.
>>> diff --git a/t/yacc-d-basic.sh b/t/yacc-d-basic.sh
>>> index 91fbc62..72872f2 100755
>>> --- a/t/yacc-d-basic.sh
>>> +++ b/t/yacc-d-basic.sh
>>> @@ -54,7 +54,14 @@ void yyerror (char *s) {}
>>> x : 'x' {};
>>> %%
>>> END
>>> -cp foo/parse.y bar/parse.y
>>> +# Using ylwrap, we actually generate y.tab.[ch].  Unfortunately, we
>>> +# forgot to rename #include "y.tab.h" into #include "parse.h" during
>>> +# the conversion from y.tab.c to parse.c.  This was OK when Bison was
>>> +# not issuing such an #include (up to 2.6).
>>>
>> A comment on this lines should also go in the commit message, as well as
>> somewhere in ylwrap (if you manage to find a place where it clearly fits).
> 
> Did so too.
> 
> Here are the two patches forked from this one.
> 
> From 3c11d8ef096f6b1a7fc894e1bd5b530f2db52f5b Mon Sep 17 00:00:00 2001
> From: Akim Demaille <address@hidden>
> Date: Fri, 13 Jul 2012 13:14:42 +0200
> Subject: [PATCH 1/2] ylwrap: rename header inclusion in generated parsers
> 
> Some types of Bison parsers, such as the GLR ones, generate a header
> file that they include.  ylwrap, which renames the generated files,
> does not rename the included file.  Fix this shortcoming, reported
> for instance here:
> <http://lists.gnu.org/archive/html/bug-bison/2012-06/msg00033.html>.
>
This explanation is very good, and IMHO decidedly improves the overall
quality of the patch.  Thanks.

> * lib/ylwrap (quote_for_sed): Accept arguments.
> Catch more special characters.
> (rename_sed): New.
> Improve the previous renaming sed commands using quote_for_sed.
> Use '|' as sed separator, instead of the historical ','.
> Suggested by Stefano Lattarini here:
> <http://lists.gnu.org/archive/html/automake-patches/2012-07/msg00095.html>.
> (main loop): Use rename_sed the rename the dependencies to other files.
> * t/yacc-d-basic.sh: Exercise this case, even if bison/yacc was
> not issuing such an include.
> ---
>  lib/ylwrap        | 23 +++++++++++++++++------
>  t/yacc-d-basic.sh |  9 ++++++++-
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/ylwrap b/lib/ylwrap
> index 8a9f2b0..48ab45c 100755
> --- a/lib/ylwrap
> +++ b/lib/ylwrap
> @@ -1,7 +1,7 @@
>  #! /bin/sh
>  # ylwrap - wrapper for lex/yacc invocations.
>  
> -scriptversion=2012-07-13.10; # UTC
> +scriptversion=2012-07-13.11; # UTC
>  
>  # Copyright (C) 1996-2012 Free Software Foundation, Inc.
>  #
> @@ -32,7 +32,7 @@ scriptversion=2012-07-13.10; # UTC
>  get_dirname ()
>  {
>    case $1 in
> -    */*|*\\*) printf '%s\n' "$1" | sed -e 's,\([\\/]\)[^\\/]*$,\1,';;
> +    */*|*\\*) printf '%s\n' "$1" | sed -e 's|\([\\/]\)[^\\/]*$|\1|';;
>      # Otherwise,  we want the empty string (not ".").
>    esac
>  }
> @@ -48,10 +48,16 @@ guard()
>          -e 's/[^ABCDEFGHIJKLMNOPQRSTUVWXYZ]/_/g'
>  }
>  
> +# quote_for_sed [STRING]
> +# ----------------------
> +# Return STRING (or stdin) quoted to be used as a sed pattern.
>  quote_for_sed ()
>  {
> -  # FIXME: really we should care about more than '.' and '\'.
> -  sed -e 's,[\\.],\\&,g'
> +  case $# in
> +    0) cat;;
> +    1) echo "$1";;
>
We'd be safer using printf, in case "$1" contains '\' characters:

   printf '%s\n' "$1"

(the Autoconf manual describes several issues with 'echo' in more details).

> +  esac \
> +    | sed -e 's|[][\\.*]|\\&|g'
>  }
>  
>  case "$1" in
> @@ -113,6 +119,10 @@ fi
>  
>  # The list of file to rename: FROM TO...
>  pairlist=
> +# A sed program to s/FROM/TO/g for all the FROM/TO so that, for
> +# instance, we rename #include "y.tab.h" into #include "parse.h"
> +# during the conversion from y.tab.c to parse.c.
> +rename_sed=
>  while test "$#" -ne 0; do
>    if test "$1" = "--"; then
>      shift
> @@ -130,6 +140,7 @@ while test "$#" -ne 0; do
>    to=$1
>    shift
>    pairlist="$pairlist $from $to"
> +  rename_sed="${rename_sed}s|"`quote_for_sed "$from"`"|$to|g;"
>  done
>  
>  # The program to run.
> @@ -196,8 +207,8 @@ if test $ret -eq 0; then
>        FROM=`guard "$from"`
>        TARGET=`guard "$to"`
>  
> -      sed -e "/^#/!b" -e "s,$input_rx,$input_sub_rx," -e "s,$from,$to," \
> -          -e "s,$FROM,$TARGET," "$from" >"$target" || ret=$?
> +      sed -e "/^#/!b" -e "s|$input_rx|$input_sub_rx|" -e "$rename_sed" \
> +          -e "s|$FROM|$TARGET|" "$from" >"$target" || ret=$?
>  
>        # Check whether header files must be updated.
>        if test $first = no; then
> diff --git a/t/yacc-d-basic.sh b/t/yacc-d-basic.sh
> index 91fbc62..72872f2 100755
> --- a/t/yacc-d-basic.sh
> +++ b/t/yacc-d-basic.sh
> @@ -54,7 +54,14 @@ void yyerror (char *s) {}
>  x : 'x' {};
>  %%
>  END
> -cp foo/parse.y bar/parse.y
> +# Using ylwrap, we actually generate y.tab.[ch].  Unfortunately, we
> +# forgot to rename #include "y.tab.h" into #include "parse.h" during
> +# the conversion from y.tab.c to parse.c.  This was OK when Bison was
> +# not issuing such an #include (up to 2.6).
> +#
> +# To make sure that we perform this conversion, in bar/parse.y, use
> +# y.tab.h instead of parse.c.
> +sed -e 's/parse\.h/y.tab.h/' <foo/parse.y >bar/parse.y
>  
>  cat > foo/main.c << 'END'
>  #include "parse.h"

As for the other patch: ACK.

Thanks,
  Stefano