Hi Joshua,
Wow, your patch is very mature, congratulations! There are even
tests!
> Le 7 mai 2020 à 18:25, Joshua Watt <jpewhac...@gmail.com> 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.
