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

[Akim Demaille]

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.


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

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

> 
>> ?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.  But I can add protections, sure.  Done below.

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


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

* 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";;
+  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"
-- 
1.7.11.2




From 73b644325412806afe2762ad20371d3507054230 Mon Sep 17 00:00:00 2001
From: Akim Demaille <address@hidden>
Date: Fri, 13 Jul 2012 13:14:44 +0200
Subject: [PATCH 2/2] ylwrap: comment changes

* lib/ylwrap: Improve some comments.
---
 lib/ylwrap | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/lib/ylwrap b/lib/ylwrap
index 48ab45c..f85a994 100755
--- a/lib/ylwrap
+++ b/lib/ylwrap
@@ -188,25 +188,22 @@ if test $ret -eq 0; then
         *) target="../$to";;
       esac
 
-      # We do not want to overwrite a header file if it hasn't
-      # changed.  This avoid useless recompilations.  However the
-      # parser itself (the first file) should always be updated,
-      # because it is the destination of the .y.c rule in the
-      # Makefile.  Divert the output of all other files to a temporary
-      # file so we can compare them to existing versions.
+      # Do not overwrite unchanged header files to avoid useless
+      # recompilations.  Always update the parser itself (the first
+      # file): it is the destination of the .y.c rule in the Makefile.
+      # Divert the output of all other files to a temporary file so we
+      # can compare them to existing versions.
       if test $first = no; 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.  Don't let the resulting
+      # debug information point at an absolute srcdir.  Use the real
+      # output file name, not yy.lex.c for instance.  Adjust the
+      # include guards too.
       FROM=`guard "$from"`
       TARGET=`guard "$to"`
-
       sed -e "/^#/!b" -e "s|$input_rx|$input_sub_rx|" -e "$rename_sed" \
           -e "s|$FROM|$TARGET|" "$from" >"$target" || ret=$?
 
-- 
1.7.11.2