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% >>> @MAINTAINER_MODE_FALSE@am__skipyacc = 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 <a...@lrde.epita.fr> > 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