Re: FYI: Fix aliases properties consistency checks

[Paul Eggert]

Akim Demaille <address@hidden> writes:

> +      if (alias->prec != orig->prec || alias->assoc != orig->assoc)
> +     if (orig->assoc != undef_assoc)
> +       symbol_precedence_set (alias, orig->prec, orig->assoc,
> +                              orig->prec_location);
> +     else
> +       symbol_precedence_set (orig, alias->prec, alias->assoc,
> +                              alias->prec_location);

Won't this generate a "redefining precedence" message if the
precedence is unchanged but the associativity is changed?
In that case, the old code complains only about conflicting
associativities, and this is nicer.

The rest of my comments are about minor style issues and the like.

> Content-Type: application/octet-stream; x-unix-mode=0644; name="diffs.patch"

It's easier for me (and I think for others) if you just include the
patch inline.  It's a text file so it shouldn't be a problem.

> Index: src/parse-gram.c
> Index: src/parse-gram.h

I regenerated those files and installed the result.

> Index: src/symtab.c
 /*-----------------------------------------------------------------.
+| Complain that S' WHAT is redeclared at SND, and was first set    |

Please replace "S'" with "S's".

In English, there are two camps about how to pluralize words whose
singular form ends in "s".  The traditionalists say to always append
"'" without the "s".  The more-modern (Fowler/Oxford) camp says to
always append "'s", except perhaps (and this is a nod to
traditionalists) for ancient names like "Jesus" and "Achilles".  There
is a similar split in pronunciation: I think most people append a
slight extra "s" sound in phrases like "Janis's house", though some
omit the "s" sound.

For computing applications, the more-modern tradition makes more
sense, as it's more consistent.  This is particularly the case here,
where I couldn't make heads or tails of what you wrote the first few
times I read it (until I thought of Jesus and Achilles :-).


+static void
+redeclaration (symbol* s, const char *what, location fst, location snd)

Plese put a space before the "*", not after, e.g., "symbol *s".  This
the GNU style, largely (I think) because it's built into the C
grammar: the * binds more tightly to the name, not to the type.

Also, please declare pointers to be "const" if possible, which is the
case here.  Also, please put the "const" after the type, not before,
as this is more consistent: in C, one can usually put "const" after a
type, but there are many cases where one cannot put "const" before.
Also, the type is usually more important than the constness, so as a
style matter it's better practice to put the type first.

Finally, abbreviations like "fst" and "snd" are a bit hard to read.
I'd prefer "first" and "second", or better yet "loc1" and "loc2".

Thus:

static void
redeclaration (symbol const *s, char const *what, location loc1, location loc2)


> +      if (alias->type_name != orig->type_name)
> +     if (orig->type_name)
> +       symbol_type_set (alias, orig->type_name, orig->type_location);
> +     else
> +       symbol_type_set (orig, alias->type_name, alias->type_location);

Please put braces around the inner if-then-else, to avoid any
dangling-else confusion (and to avoid complaints from GCC :-).
Similarly for other places where this construct appears.
(The braces were in the orignial version, for this reason.)

> +      symbol *alias = this;
> +      symbol *orig  = this->alias;

This is a very minor point, but I've found that attempting to line up
"=" like this is generally counterproductive.  It is a bit of a
distraction to the reader (to this reader, at least), who ends up
wondering if the "="s are being lined up specially for some reason.
And when taken to extremes (which of course this is not) it can take
away valuable screen real estate.