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

[Adrian Vogelsgesang]

Hi Akim,

> Yes, in m4 you don't have the choice. 'm4_foo (' is the same
> as 'm4_foo() ('.
Just learned something new about m4

> > Test case 493 was complaining about the variable ` lvalp` being unused.
> I addressed that in the first patch, that prepares the test suite.
Thanks! I just took a look at it and it would have taken me quite some time to
find the solution `AT_YYLEX_DEFINE`.


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

> There's an issue I had not paid attention to: you made a lac_stack_
> a fully blown stack of symbols.
I totally missed that. Thanks for catching it, this was unintended.

> 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 - quite frankly: I am not sure the
`stack` abstraction adds much value in the first place.

I will try to address those issues during this week - not sure when
exactly I will find the time for it.

Cheers,
Adrian

On 14/07/2019, 13:55, "Akim Demaille" <address@hidden> wrote:

    There's an issue I had not paid attention to: you made a lac_stack_
    a fully blown stack of symbols (i.e., type, value and location).  I
    believe you only need a stack a states, the rest is useless.  And
    GCC 4.6 chokes on it :)
    
    But stack.hh was really designed for symbols, not simple values such
    as state numbers.  Which is going to be painful to have work properly
    in C++98.  I tried to have it work properly, but C++98 is really
    painful (I had forgotten default template parameters were not allowed
    for function templates, only class template).
    
    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_.
    If you do so, please also remove stack::empty and other now-useless
    membres.
    
    
    diff --git a/data/skeletons/lalr1.cc b/data/skeletons/lalr1.cc
    index 60c2ba71..5f269666 100644
    --- a/data/skeletons/lalr1.cc
    +++ b/data/skeletons/lalr1.cc
    @@ -369,7 +369,7 @@ m4_define([b4_shared_declarations],
         /// yy_lac_check_. We just store it as a member of this class to hold
         /// on to the memory and to avoid frequent reallocations.
         /// Since yy_lac_check_ is const, this member must be mutable.
    -    mutable stack<by_state> yylac_stack_;
    +    mutable stack<state_type> yylac_stack_;
         /// Was an initial LAC context established?
         bool yy_lac_established_;
     ]])[
    @@ -1142,9 +1142,8 @@ b4_dollar_popdef])[]dnl
         size_t lac_top = 0;
         while (true)
           {
    -        state_type top_state = (yylac_stack_.empty ()
    -                                ? yystack_[lac_top].state
    -                                : yylac_stack_[0].state);
    +        state_type top_state =
    +          yylac_stack_.empty () ? yystack_[lac_top].state : 
yylac_stack_[0];
             int yyrule = yypact_[top_state];
             if (yy_pact_value_is_default_ (yyrule)
                 || (yyrule += yytoken) < 0 || yylast_ < yyrule
    @@ -1196,13 +1195,12 @@ b4_dollar_popdef])[]dnl
               lac_top += yylen;
             }
             // Keep top_state in sync with the updated stack.
    -        top_state = (yylac_stack_.empty ()
    -                     ? yystack_[lac_top].state
    -                     : yylac_stack_[0].state);
    +        top_state =
    +          (yylac_stack_.empty () ? yystack_[lac_top].state : 
yylac_stack_[0]);
             // Push the resulting state of the reduction.
             state_type state = yy_lr_goto_state_ (top_state, yyr1_[yyrule]);
             YYCDEBUG << " G" << state;
    -        yylac_stack_.push (by_state (state));
    +        yylac_stack_.push (state_type (state));
           }
       }
     
    diff --git a/data/skeletons/stack.hh b/data/skeletons/stack.hh
    index a481fb26..5355559f 100644
    --- a/data/skeletons/stack.hh
    +++ b/data/skeletons/stack.hh
    @@ -26,7 +26,19 @@ b4_defines_if([b4_required_version_if([302], [],
     # b4_stack_define
     # ---------------
     m4_define([b4_stack_define],
    -[[    /// A stack with random access from its top.
    +[[    template <class T, class U>
    +      struct is_same { enum {value = false}; };
    +
    +    template <class T>
    +      struct is_same<T, T> { enum{value = true}; };
    +
    +    template <bool B, typename T = void>
    +    struct enable_if {};
    +
    +    template <typename T>
    +    struct enable_if <true, T> { typedef T type; };
    +
    +    /// A stack with random access from its top.
         template <typename T, typename S = std::vector<T> >
         class stack
         {
    @@ -76,10 +88,18 @@ m4_define([b4_stack_define],
             return operator[] (size_type (i));
           }
     
    +      template <typename U = T>
    +      typename enable_if<is_same<U, int>::value>::type
    +      push (int t)
    +      {
    +        seq_.push_back (t);
    +      }
    +
           /// Steal the contents of \a t.
           ///
           /// Close to move-semantics.
    -      void
    +      template <typename U = T>
    +      typename enable_if<!is_same<U, int>::value>::type
           push (YY_MOVE_REF (T) t)
           {
             seq_.push_back (T ());