Hi Adrian, > Le 15 juil. 2019 à 13:23, Adrian Vogelsgesang <[email protected]> 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!
