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

[Akim Demaille]

Hi Joshua,

Wow, your patch is very mature, congratulations!  There are even
tests!

> Le 7 mai 2020 à 18:25, Joshua Watt <address@hidden> a écrit :
> 
> Teaches bison about a new command line option, --file-prefix-map OLD=NEW
> 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.

This sounds desirable, indeed.  It has always bugged me that the generated
files were so dependent on the user's environment.  In fact Bison itself
is cheating (have a look at the bottom of tests/bison.in).

> 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

It seems weird to map "foo/" to "bar".

> Importantly, this will change the header guards from:
> 
> #ifndef YY_BUILD_BISON_PARSE_H
> 
> to
> 
> #ifndef YY_USR_SRC_BISON_PARSE_H
> 
> which is reproducible.

It appears that your approach is not sufficient for synclines.  I toyed
with it, and the generated #lines are not adjusted.

I'm somewhat bugged that this relies on a new option, mostly because it
means that it cannot be controlled from within the grammar file itself.
Do you really think we need a list of rules?  Do you have evidence for
that need?

If not, maybe we can find a solution which would rely only on %define/-D.


> diff --git a/src/files.c b/src/files.c
> index 71c10e34..f685cdaa 100644
> --- a/src/files.c
> +++ b/src/files.c
> /* C source file extension (the parser source).  */
> static char *src_extension = NULL;
> /* Header file extension (if option '`-d'' is specified).  */
> static char *header_extension = NULL;
> +
> +static struct prefix_map {
> +  struct prefix_map *next;
> +  char *oldprefix;
> +  char *newprefix;
> +} *prefix_maps;

Please separate the variable definition from the struct
definition.

We are now getting rid of our hand-written list structures
in favor gnulib's implementation of lists.  Not much of a big deal,
but I'd rather avoid stepping backward at this point.


> +static char *
> +map_file_name (char const *filename)
> +{
> +  struct prefix_map const *p;
> +
> +  if (!filename)
> +    return NULL;

We're in C99, so keep the scopes as small as possible: move if before
the introduction of p.

> +  for (p = prefix_maps; p != NULL; p = p->next)
> +    if (strncmp(p->oldprefix, filename, strlen(p->oldprefix)) == 0)

We put spaces before parens.

> +      break;
> +
> +  if (!p)
> +    return xstrdup (filename);
> +
> +  size_t oldprefix_len = strlen (p->oldprefix);
> +  size_t newprefix_len = strlen (p->newprefix);
> +  char *s = xmalloc (1 + newprefix_len + strlen (filename) - oldprefix_len);

I'd prefer the 1 to be last.


> +  strcpy (s, p->newprefix);
> +  strcat (s, &filename[oldprefix_len]);

We use stpcpy for this kind of things.

Out of curiosity, why do you prefer "&filename[oldprefix_len]" to
"filename + oldprefix_len"?  The latter seems more natural to me.


> +
> +  return s;
> +}
> +
> +void
> +add_prefix_map(char const* oldprefix, char const* newprefix)
> +{
> +  struct prefix_map *p = xmalloc(sizeof(*p));
> +  p->oldprefix = xstrdup(oldprefix);
> +  p->newprefix = xstrdup(newprefix);

Many space-before-paren issues.  Elsewhere too.



> +  while (prefix_maps) {

Braces alone on their lines please.

> +    struct prefix_map *p = prefix_maps;
> +    prefix_maps = prefix_maps->next;
> +    free(p->oldprefix);
> +    free(p->newprefix);
> +  }
> }
> diff --git a/src/files.h b/src/files.h
> index 00814ad0..379e6495 100644
> --- a/src/files.h
> +++ b/src/files.h
> @@ -49,9 +49,11 @@ extern char *spec_xml_file;
> 
> /* File name specified with --defines.  */
> extern char *spec_header_file;
> +extern char *spec_mapped_header_file;

Please, document a bit.

> @@ -765,6 +769,18 @@ getargs (int argc, char *argv[])
>         no_lines_flag = true;
>         break;
> 
> +      case 'M': // -MOLDPREFIX=NEWPREFIX

I would prefer ":", which is more natural to my eyes to denote a
mapping.

> +        {

Useless braces here.

> +          if (optarg) {
> +            char *newprefix = strchr (optarg, '=');
> +            if (newprefix) {

paren and brace issues.

> +              *newprefix = 0;

Please use '\0'.  We did use 0 to mean char 0 in many places, but I'm
moving to using '\0' instead.

> +              add_prefix_map(optarg, newprefix + 1);
> +            }
> +          }
> +        }
> +        break;
> +
>       case 'o':
>         spec_outfile = optarg;
>         break;



> diff --git a/tests/c++.at b/tests/c++.at
> index 490c6c25..cdc2a0d4 100644
> --- a/tests/c++.at
> +++ b/tests/c++.at
> @@ -1489,7 +1489,100 @@ AT_PARSER_CHECK([parser], [0])
> 
> AT_CLEANUP
> 
> +## --------------------------- ##
> +## Header Guard Prefix Mapping ##
> +## --------------------------- ##
> +
> +AT_SETUP([Header Guard Prefix Mapping])
> +
> +# AT_TEST([PREFIX], [DIRECTIVES])

I wouldn't put this test here, as we should also check the other languages.

Have you seen output.at?  I think it would be more natural.
Maybe we can even reuse easily the existing tests, like AT_CHECK_OUTPUT.

I can address this later.