Re: Lookahead correction for the C++ skeleton lalr1.cc

[Akim Demaille]

Hi Adrian,

> Le 15 juil. 2019 à 13:23, Adrian Vogelsgesang <address@hidden> a écrit :
> 
>> I see you made new changes, in particular the use of unsigned types
>> for sizes. Well, I used to follow that path (if it's nonnegative,
>> make it unsigned), but I changed my mind. In the C++ community, people
>> tend to shy away from unsigned except for bit fields. They prefer
>> signed types because wrapping is undefined, and therefore i. is faster.
>> ii. can checked by tools for UB. Unsigned types on the other hand
>> have well defined wrapping behavior, which is seldom what you actually
>> need (an error would be better).
>> 
>> To the point that I believe they would have used 'int' everywhere, even
>> for size().
>> 
>> stack.hh is already featuring too much of that hesitation, I'd prefer
>> sticking to ints.
> 
> I did those changes to pacify `-Wsign-compare` and friends. I agree that
> `int` might be preferable for performance reasons. I see two ways to
> change it back to int: 1) by using `static_cast` directly in lalr1.cc to 
> silence
> the warnings 2) to put that `static_cast` into `stack.hh`. Which one would
> you prefer? (in case we decide to stick to using stack.hh at all)

I like to push internal details down, so the fixes should rather
be in stack (but I don't think we want to use it here).

>> But stack.hh was really designed for symbols, not simple values such
>> as state numbers. You may play with the appended protopatch if you
>> feel like it, but otherwise I would just go for a plain std::vector for
>> lac_stack_.
> Personally, I would prefer std::vector

Yes, let's use it here.

> - quite frankly: I am not sure the
> `stack` abstraction adds much value in the first place.

The point was to present just the right slice of data to the action.
Maybe it was over-engineered, but it works and is quite elegant.  Besides,
IIRC first iterations of lalr1.cc did have several stacks, the unification
as a single concept of "symbol" happened later.  So at some point it
probably helped factoring more things.

Today it might be totally useless.  If you want to propose a patch
that removes it, we would certainly install it.


Cheers!