Re: FYI: Adjust @$ for empty reductions

[Paul Eggert]

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

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;

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.

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

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

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

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.

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?