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

[Stefano Lattarini]

Hi Akim.

Thanks for tackling and fixing these issues.  The patch is basically
OK, although I have few nits and question below, that I'd like to be
addressed before giving an ACK.

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

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

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

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

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

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

> [SNIP] the rest of the patch.

Thanks,
  Stefano