[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Lookahead correction for the C++ skeleton lalr1.cc
From: |
Akim Demaille |
Subject: |
Re: Lookahead correction for the C++ skeleton lalr1.cc |
Date: |
Tue, 16 Jul 2019 18:32:07 +0200 |
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!
- Lookahead correction for the C++ skeleton lalr1.cc, Adrian Vogelsgesang, 2019/07/09
- Re: Lookahead correction for the C++ skeleton lalr1.cc, Akim Demaille, 2019/07/09
- Re: Lookahead correction for the C++ skeleton lalr1.cc, Adrian Vogelsgesang, 2019/07/09
- Re: Lookahead correction for the C++ skeleton lalr1.cc, Akim Demaille, 2019/07/13
- Re: Lookahead correction for the C++ skeleton lalr1.cc, Adrian Vogelsgesang, 2019/07/13
- Re: Lookahead correction for the C++ skeleton lalr1.cc, Akim Demaille, 2019/07/14
- Re: Lookahead correction for the C++ skeleton lalr1.cc, Akim Demaille, 2019/07/14
- Re: Lookahead correction for the C++ skeleton lalr1.cc, Adrian Vogelsgesang, 2019/07/15
- Re: Lookahead correction for the C++ skeleton lalr1.cc,
Akim Demaille <=