[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: FYI: Fix aliases properties consistency checks

From: Akim Demaille
Subject: Re: FYI: Fix aliases properties consistency checks
Date: Sun, 10 Oct 2004 17:45:47 +0200

Le 9 oct. 04, à 20:53, Paul Eggert a écrit :

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?

You can't change one without the other: they're bound together.
That's one of the things I'd like to change, but that's another

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.

I don't control the wrapping with this mailer.  That's why I didn't
do as I always do.

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

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

Thanks for the details!

+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.

Yeah, I know.  C and C++ suck in this regard.  But note that in this
case, argument declaration, the "real" grammar has no visible impact.
Only in variable declarations.

Anyway, this was a mistake.  Like the obviously missing const.

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.

Well, that's new.  I'll stick to const symbol, because I'm used to
reading const char in most GNU programs I read.  But maybe it's
been too long I last read C sources.

Given that bison is written with const type, not type const, I'll
stay consistent with it.

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

Well, I used fst and snd because once Paul Eggert changed all my
identifiers to something shorter.  It was to avoid such a comment
that I used fst and snd.  Which are widely used IMHO.  But I'll
use first and second, which I prefer given the message the routine

+      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.)

It's heavy style, but ok.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]