Re: [PATCH v2] Add command line option to map file prefixes

[Akim Demaille]

Hi Joshua,

> Le 11 mai 2020 à 04:00, Joshua Watt <address@hidden> a écrit :
> 
> Teaches bison about a new command line option, --file-prefix-map OLD=NEW
> (based on the -fdebug-prefix-map option from GCC)

Why don't you refer to the same option, -ffile-prefix-map=old=new 
(https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html)?

> which causes it to
> replace and file path of OLD in the text of the output file with NEW,
> mainly for header guards and comments. The primary use of this is to
> make builds reproducible with different input paths, and in particular
> the debugging information produced when the source code is compiled. For
> example, a distro may know that the bison source code will be located at
> "/usr/src/bison" and thus can generate bison files that are reproducible
> with the following command:
> 
> bison --output=/build/bison/parse.c -d -L C 
> --file-prefix-map=/build/bison/=/usr/src/bison/ parse.y
> 
> Importantly, this will change the header guards and #line directives
> from:
> 
> #ifndef YY_BUILD_BISON_PARSE_H
> #line 100 /build/bison/parse.h

You should put the quotes too.

> 
> to
> 
> #ifndef YY_USR_SRC_BISON_PARSE_H
> #line 100 /usr/src/bison/parse.h
> 
> which is reproducible.

This looks great!  You still have a few missing space-before-parens.

> diff --git a/src/files.h b/src/files.h
> index 00814ad0..1f943562 100644
> --- a/src/files.h
> +++ b/src/files.h
> @@ -82,4 +88,7 @@ FILE *xfopen (const char *name, char const *mode);
> void xfclose (FILE *ptr);
> FILE *xfdopen (int fd, char const *mode);
> 
> +char *map_file_name (char const *filename);
> +void add_prefix_map(char const* oldprefix, char const* newprefix);

Please document (even though there's not must to say, I agree).

> +      case 'M': // -MOLDPREFIX=NEWPREFIX
> +        if (optarg)
> +          {
> +            char *newprefix = strchr (optarg, '=');
> +            if (newprefix)
> +              {
> +                *newprefix = '\0';
> +                add_prefix_map (optarg, newprefix + 1);
> +              }
> +          }

We ought to complain about bad arguments.

On second thought maybe everything would have been easier if we had kept the 
list of -M arguments ("OLDPREFIX=NEWPREFIX"), and split them when we actually 
map a file name.  Less boilerplate, in particular for memory management.  Not a 
big deal.

> +# Check the CPP guard and Doxyen comments.
> +AT_CHECK([sed -ne 's/#line [[:digit:]]* /#line 
> /p;/INCLUDED/p;/\\file/{p;n;p;}' out/include/ast/loc.hh],

I don't think we can trust the portability of [:digit:], let's go with [0-9].  
And I'd prefer that we see that there were actual line numbers, like `s/#line 
[0-9][0-9]* /#line XXX/p`.




This is really great!  I'll install this as soon as I get the agreement from 
the copyright clerk of the FSF.  Meanwhile, if you want to wrap that feature 
completely, we need an update of the documentation and an entry in NEWS.  Also, 
I'd like to get rid of the ugly hack in tests/bison.in, but that might require 
changing the build system to stop using the default rules of Automake, so I can 
deal with that afterwards.

Cheers!