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.