Re: FYI: Adjust @$ for empty reductions

[Akim Demaille]

Le 5 oct. 04, à 20:19, Paul Eggert a écrit :

> Akim Demaille <address@hidden> writes:
>
> > +# define YYLLOC_DEFAULT(Current, Rhs, N)                            \
> > +do {                                                                \
> > +  if (N)                                                            \
> > +    {                                                       \
> > +      (Current).first_line   = (Rhs)[1].first_line;                 \
> > +      (Current).first_column = (Rhs)[1].first_column;       \
> > +      (Current).last_line    = (Rhs)[N].last_line;                  \
> > +      (Current).last_column  = (Rhs)[N].last_column;                \
> > +    }                                                       \
> > +  else                                                      \
> > +    {                                                       \
> > +      (Current).first_line   = (Current).last_line   = 
> > (Rhs)[0].last_line;  \
> > +      (Current).first_column = (Current).last_column = 
> > (Rhs)[0].last_column;\
> > +    }                                                       \
> > +} while(0)
>
> This doesn't look right to me.  A location whose first and last
> columns are the same is not an empty location: it is a location that
> contains a single character.  (This issue did not arise in
> src/parse-gram.y, as it uses a definition of locations in which the
> upper bound is exclusive, not inclusive.)

I don't read locations as you do.  For me positions (two of which
are referring to the sapce in between characters.  The first
position is immediately pointing before the first character, and
the second is immediately the last.

An empty location is a location such that begin = end.

> It seems to me that the last assignment should look like something
> like this instead:
>
>    (Current).last_column = (Rhs)[0].last_column;
>    (Current).first_column = (Current).last_column + 1;

I really don't agree.  You're designating a single character here.
Locations are intervals, an empty interval has equal boundaries.  If
boundaries differs, there is something in it.

> A few other things.  The patch replaces an expression with a statement
> (omitting its trailing ";").  This isn't an upward-compatible change,
> as expressions can be used in contexts that statements can't.

It has never been documented as such, and frankly, I don't think it
matters at all.  It should never have been an expression in the first
place.

> I suspect that you'll get more-efficient code if you revamp the macro
> to merge duplicate code.  Certainly the code will be shorter.

That's the compiler's job, not mine.

> Some minor nits: you should use GNU style for indenting and spacing.

Hm.  I should double checked, but I did not mean to escape from GNU
style.  What did I miss?

> All things considered, how about this definition instead?
>
> # define YYLLOC_DEFAULT(Current, Rhs, N)                        \
>    ((Current).last_line    = (Rhs)[N].last_line,                \
>     (Current).last_column  = (Rhs)[N].last_column,              \
>     ((N)                                                        \
>      ? ((Current).first_line   = (Rhs)[1].first_line,           \
>         (Current).first_column = (Rhs)[1].first_column)         \
>      : ((Current).first_line   = (Current).last_line,           \
>         (Current).first_column = (Current).last_column + 1)))

I don't see the point: it is less readable IMHO, and I don't
like the final + 1.  Also, it is less "open" to additional
changes (such as those we refer to below). Sure, it's an
expression, but does it keeps the same value as it did
before the change?

I doubt it, and I think nobody cares.  I'm against
carving into stone this idea of making it an expression.

> Similarly for glr.c and lalr1.cc.
>
> One other thing: the documentation needs to be updated, as it contains
> a copy of the old macro definition.

I agree, and NEWS too.  But I have not finished.

> And while we're on the subject, are you intending to modify
> YYLLOC_DEFAULT to ignore empty locations at the start of the right
> hand side, as src/parse-gram.y's lloc_default does?

Yep.