[Top][All Lists]

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

Re: FYI: Adjust @$ for empty reductions

From: Akim Demaille
Subject: Re: FYI: Adjust @$ for empty reductions
Date: Tue, 5 Oct 2004 22:07:10 +0200

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

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

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?


reply via email to

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