Re: [PATCH 2/2] ylwrap: various fixes

[Akim Demaille]

Le 21 déc. 2012 à 17:44, Stefano Lattarini <address@hidden> a écrit :

> Hi Akim.

Hi!

> On 12/19/2012 02:55 PM, Akim Demaille wrote:
>> * lib/ylwrap (guard): Properly honor $1.
>> 
> I fear this is the ChangLog style that I dislike: just reporting the
> "what" of the change, without the "why".

The "why" here is obvious: because it was wrong.  If you
think "Fix a typo" is better, I sure can.  But I see no
point in stating a better why (the title is there).
IMHO, "properly" suffices.

But I extended below.

>  Since you have a very good
> explanation of such a "why" (as seen in the NEWS file), would you
> mind reporting it (at least in an abridged form) in the commit message
> as well?

If you think it's useful, I sure can.

>> Keep a single _ instead of several.
>> (RENAME_sed): new.
>> 
> Micro-nit: Missing capitalization of "new".

Ok.

> Major nit: I really dislike having tow variable whose names only
> differ in capitalization.  How about renaming them like follows?
> 
>  rename_sed -> sed_fix_filenames
>  RENAME_sed -> sed_fix_header_guards

As you like :)

> You can squash the rename in this patch, or do it with a follow-up,
> as you prefer.
> 
>> Use it.
>> ---
>> NEWS       | 18 ++++++++++++++++++
>> lib/ylwrap | 27 ++++++++++++++++++---------
>> 2 files changed, 36 insertions(+), 9 deletions(-)
>> 
>> diff --git a/NEWS b/NEWS
>> index 7a230ef..482216c 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -1,3 +1,21 @@
>> +New in 1.12.7:
>> +
>> +Bugs fixed in 1.12.7:
>> +
>> 
> I don't plan to actually make a further release in the 1.12.x series
> for such a bug.  That is, unless it is causing some serious problems
> that I've been missing (in which case, they should be reported more
> prominently in the commit message); is this the case?  If not, just
> rebase your series on master, and report the news as "New in 1.13".

I misunderstood your previous message and thought you wanted
this on maint.  I moved it to master.

>> +  - ylwrap renames properly header guards in generated header files,
>> +    instead of leaving Y_TAB_H.
>> +
>> +  - ylwrap now also converts header guards in implementation files.
>> 
> Sorry if I sound dense, but what are exactly these "implementation
> files"?

*.c vs *.h.

> 
>> +    Because ylwrap failed to rename properly #include in the
>> +    implementation files, current versions of Bison (e.g., 2.7)
>> +    duplicate the generated header file in the implementation file.
>> +    The header guard then protects the implementation file from
>> +    duplicate definitions from the header file.
>> +
>> +  - ylwrap generates header guards with a single '_' for series of non
>> +    alphabetic characters, instead of several.  This is what Bison
>> +    does.
>> 
> This is more a (justified) change in behaviour than a bug; it should
> be reported as that, IMHO.

OK.

commit 5fa0ddb63477a70808d0723b1fc3f0cdb8f7f145
Author: Akim Demaille <address@hidden>
Date:   Wed Dec 19 14:30:06 2012 +0100

    tests: strengthen the ylwrap tests
    
    * t/yacc-d-basic.sh: Comment changes.
    (generated): New.
    Use it to factor various tests.
    Check that Y_TAB_H is not issued.

diff --git a/t/yacc-d-basic.sh b/t/yacc-d-basic.sh
index 1d2fc07..0067b7d 100755
--- a/t/yacc-d-basic.sh
+++ b/t/yacc-d-basic.sh
@@ -59,8 +59,10 @@ END
 # 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.
+# To make sure that we perform this conversion even with version of
+# Bison that do not generate this include, in bar/parse.y, use y.tab.h
+# instead of parse.h, and check the ylwrap does replace "y.tab.h" with
+# "parse.h".
 sed -e 's/parse\.h/y.tab.h/' <foo/parse.y >bar/parse.y
 
 cat > foo/main.c << 'END'
@@ -107,12 +109,15 @@ $AUTOMAKE baz/Makefile
 
 $MAKE
 
-test -f foo/parse.c
-test -f foo/parse.h
-test -f bar/parse.c
-test -f bar/parse.h
-test -f baz/zardoz-parse.c
-test -f baz/zardoz-parse.h
+generated="foo/parse.c foo/parse.h bar/parse.c bar/parse.h baz/zardoz-parse.c
+baz/zardoz-parse.h"
+
+for i in $generated
+do
+  test -f $i
+  # There must remain no obsolete header guard.
+  ! grep Y_TAB_H $generated
+done
 
 # The generated C source and header files must be shipped.
 for dir in foo bar; do
@@ -130,12 +135,10 @@ cd ..
 
 $MAKE distdir
 ls -l $distdir
-test -f $distdir/foo/parse.c
-test -f $distdir/foo/parse.h
-test -f $distdir/bar/parse.c
-test -f $distdir/bar/parse.h
-test -f $distdir/baz/zardoz-parse.c
-test -f $distdir/baz/zardoz-parse.h
+for i in $generated
+do
+  test -f $distdir/$i
+done
 
 # Sanity check the distribution.
 yl_distcheck
@@ -143,19 +146,15 @@ yl_distcheck
 # While we are at it, make sure that 'parse.c' and 'parse.h' are erased
 # by maintainer-clean, and not by distclean.
 $MAKE distclean
-test -f foo/parse.c
-test -f foo/parse.h
-test -f bar/parse.c
-test -f bar/parse.h
-test -f baz/zardoz-parse.c
-test -f baz/zardoz-parse.h
+for i in $generated
+do
+  test -f $i
+done
 ./configure # Re-create 'Makefile'.
 $MAKE maintainer-clean
-test ! -e foo/parse.c
-test ! -e foo/parse.h
-test ! -e bar/parse.c
-test ! -e bar/parse.h
-test ! -e baz/zardoz-parse.c
-test ! -e baz/zardoz-parse.h
+for i in $generated
+do
+  test ! -e $i
+done
 
 :

commit 2af6bc0c5f3d19b2a39fa22166094dc42d8dec2f
Author: Akim Demaille <address@hidden>
Date:   Wed Dec 19 14:51:58 2012 +0100

    ylwrap: various fixes
    
    rename properly header guards in generated header files, instead of
    leaving Y_TAB_H.
    
    convert header guards in implementation files.  Because ylwrap failed
    to rename properly #include in the implementation files, current
    versions of Bison (e.g., 2.7) duplicate the generated header file in
    the implementation file.  The header guard then protects the
    implementation file from duplicate definitions from the header file.
    
    generate header guards with a single '_' for series of non alphabetic
    characters, instead of several.  This is what Bison does.
    
    Fixes t/yacc-d-basic.sh.
    
    * lib/ylwrap (guard): Properly honor $1 to rename properly the
    header guards.
    Keep a single _ instead of several.
    (rename_sed): Rename as...
    (sed_fix_filenames): this.
    Suggested by Stefano Lattarini.
    (sed_fix_header_guards): New.
    Use it.

diff --git a/NEWS b/NEWS
index 5bf4379..d827798 100644
--- a/NEWS
+++ b/NEWS
@@ -49,6 +49,18 @@ New in 1.13:
     should take precedence over the same-named automake-provided macro
     (defined in '/usr/local/share/aclocal-1.14/vala.m4').
 
+* Bugs fixed:
+
+  - ylwrap renames properly header guards in generated header files
+    (*.h), instead of leaving Y_TAB_H.
+
+  - ylwrap now also converts header guards in implementation files
+    (*.c).  Because ylwrap failed to rename properly #include in the
+    implementation files, current versions of Bison (e.g., 2.7)
+    duplicate the generated header file in the implementation file.
+    The header guard then protects the implementation file from
+    duplicate definitions from the header file.
+
 * Version requirements:
 
   - Autoconf 2.65 or greater is now required.
@@ -206,6 +218,12 @@ New in 1.13:
   - Support for tcc (the Tiny C Compiler) has been improved, and is now
     handled through a dedicated 'tcc' mode.
 
+* The ylwrap script:
+
+  - ylwrap generates header guards with a single '_' for series of non
+    alphabetic characters, instead of several.  This is what Bison
+    does.
+
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 Bugs fixed in 1.12.6:
diff --git a/lib/ylwrap b/lib/ylwrap
index 7befa46..b5c673d 100755
--- a/lib/ylwrap
+++ b/lib/ylwrap
@@ -1,7 +1,7 @@
 #! /bin/sh
 # ylwrap - wrapper for lex/yacc invocations.
 
-scriptversion=2012-07-14.08; # UTC
+scriptversion=2012-12-21.17; # UTC
 
 # Copyright (C) 1996-2012 Free Software Foundation, Inc.
 #
@@ -42,10 +42,11 @@ get_dirname ()
 # The CPP macro used to guard inclusion of FILE.
 guard()
 {
-  printf '%s\n' "$from" \
-    | sed \
-        -e 'y/abcdefghijklmnopqrstuvwxyz/ABCDEFGHIJKLMNOPQRSTUVWXYZ/'\
-        -e 's/[^ABCDEFGHIJKLMNOPQRSTUVWXYZ]/_/g'
+  printf '%s\n' "$1"                                                    \
+    | sed                                                               \
+        -e 'y/abcdefghijklmnopqrstuvwxyz/ABCDEFGHIJKLMNOPQRSTUVWXYZ/'   \
+        -e 's/[^ABCDEFGHIJKLMNOPQRSTUVWXYZ]/_/g'                        \
+        -e 's/__*/_/g'
 }
 
 # quote_for_sed [STRING]
@@ -121,10 +122,16 @@ fi
 # The parser itself, the first file, is the destination of the .y.c
 # rule in the Makefile.
 parser=$1
+
 # 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=
+sed_fix_filenames=
+
+# Also rename header guards, as Bison 2.7 for instance uses its header
+# guard in its implementation file.
+sed_fix_header_guards=
+
 while test "$#" -ne 0; do
   if test "$1" = "--"; then
     shift
@@ -141,7 +148,8 @@ while test "$#" -ne 0; do
   shift
   to=$1
   shift
-  rename_sed="${rename_sed}s|"`quote_for_sed "$from"`"|$to|g;"
+  sed_fix_filenames="${sed_fix_filenames}s|"`quote_for_sed "$from"`"|$to|g;"
+  sed_fix_header_guards="${sed_fix_header_guards}s|"`guard "$from"`"|"`guard 
"$to"`"|g;"
 done
 
 # The program to run.
@@ -174,7 +182,7 @@ ret=$?
 if test $ret -eq 0; then
   for from in *
   do
-    to=`printf '%s\n' "$from" | sed "$rename_sed"`
+    to=`printf '%s\n' "$from" | sed "$sed_fix_filenames"`
     if test -f "$from"; then
       # If $2 is an absolute path name, then just use that,
       # otherwise prepend '../'.
@@ -197,10 +205,11 @@ if test $ret -eq 0; then
       # 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=$?
+      sed -e "/^#/!b"                           \
+          -e "s|$input_rx|$input_sub_rx|"       \
+          -e "$sed_fix_filenames"               \
+          -e "$sed_fix_header_guards"           \
+        "$from" >"$target" || ret=$?
 
       # Check whether files must be updated.
       if test "$from" != "$parser"; then