Re: FYI: Fix aliases properties consistency checks

[Akim Demaille]

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

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

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