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

[Stefano Lattarini]

On 07/12/2012 03:51 PM, Akim Demaille wrote:
> * lib/am/yacc.am (am__yacc_c2h): Shorten.
>
See below.

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

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

The HACKING file in the Automake tree gives more details:

  * Here is the general format that Automake's commit messages are expected
    to follow.  See the further points below for clarifications and minor
    corrections.

        topic: brief description (this is the "summary line")

        <reference to relevant bugs, if any>

        Here goes a more detailed explanation of why the commit is needed,
        and a general overview of what it does, and how.  This section is
        optional, but you are expected to provide it more often than not.

        And if the detailed explanation is quite long or detailed, you
        can want to break it in more paragraphs.

        Then you can add references to relevant mailing list discussions
        (if any), with proper links.  But don't take this as an excuse for
        writing incomplete commit messages!  The "distilled" conclusions
        reached in such discussions should have been placed in the
        paragraphs above.

        Finally, here you can thank people that motivated or helped the
        change.  So, thanks to John Doe for bringing up the issue, and to
        J. Random Hacker for providing suggestions and testing the patch.

        <detailed list of touched files>

> * t/yacc-d-basic.sh: Exercize
>
s/ize/ise/

> this case, even if bison/yacc was not issueing such an include.
> ---
>  lib/am/yacc.am    |  3 +--
>  lib/ylwrap        | 15 ++++++++-------
>  t/yacc-d-basic.sh |  9 ++++++++-
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/am/yacc.am b/lib/am/yacc.am
> index e74259f..fa0dc4c 100644
> --- a/lib/am/yacc.am
> +++ b/lib/am/yacc.am
> @@ -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.

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

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

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


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

> +#
> +# 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"

Thanks,
  Stefano