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

[Akim Demaille]

Hi Adrian!

> Le 13 juil. 2019 à 14:50, Adrian Vogelsgesang <address@hidden> a écrit :
> 
> You can find the updated commits in https://github.com/akimd/bison/pull/14. 
> I also attached the commits to this email, so they are archived outside
> of github.

You did well: the real place to publish patch is here.  GitHub is handy,
but auxiliary.

I have put your changes "back" to "my" branch, Travis is running currently.

> Unfortunately, it seems that the Travis config is broken for pull-requests 
> from
> forks.

Yes, there are a couple of secrets there (license for ICC and password
to upload/download the tarballs) that Travis does not want to leak.  So,
in case your PR would change something that would reveal my secrets in
your logs, they just remove the secrets.

> I think the comment in your commit should read
> > parse.lac.* options are useless in C++ even if LAC is actually activated.
> instead of
> > parse.lac.* options are useless in C++ even if LAC isn't actually activated.

Actually no: these options make sense in C only when LAC is enabled,
but in C++, they are never used.


> To your points:
> 
> > - please separate the part about yacc.c
> done. Feel free to do whatever you want with the `yacc.c` changes.
> I made them while I was reading the yacc.c LAC support, and since
> I never used m4 before, to me code without m4 branches is easier to
> Read. But since this is primarily my personal preference, feel free to
> drop the changes to yacc.c in case you don’t like them.

Your change makes sense, I'm fine with it.


> > - stay within 76 columns, at most 79 occasionally
> Done – for the most part. I didn’t know how to break the comment in 
> ` yysyntax_error_` such that the ` b4_lac_if` doesn’t go over the 79
> columns without interfering with the whitespace in the generated C++
> file.

You are right that the limit is somewhat fuzzy when it comes to
before/after m4 :)


> > - space before parens
> Totally missed that...
> Done for the C++-code (not the m4 code).
> From reading the rest of `lalr1.cc`, I assume it’s still `b4_lac_if(`
> instead of ` b4_lac_if (`.

Yes, in m4 you don't have the choice.  'm4_foo (' is the same
as 'm4_foo() ('.


> > - enable all the warnings and make sure there are no errors
> > when running the test suite. See configure --enable-gcc-warnings,
> > see also https://travis-ci.org/akimd/bison/builds/558164975
> done. Most error were due to me using yy_token_number instead of int.
> I switched over to int, and all test cases except for “493 LAC: Exploratory
> stack” passed.
> Test case 493 was complaining about the variable ` lvalp` being unused.

I addressed that in the first patch, that prepares the test suite.


> > - there's a couple of useless "#if YYDEBUG" around a "YYCDEBUG"
> The `#if YYDEBUG` is needed since the `YYCDEBUG` line accesses `yytname_`
> and `yytname_` itself is wrapped into a `#if YYDEBUG`.

Aaah, of course.  Thanks!


> > - please match the style of the git commit messages.
> added the list of changed files to the commit message and adjusted
> the subject lines. Not sure if I missed anything else.

I tricked your message a bit.


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.


FTR, I made this changes on top of yours.  Feel free to squash them
in yours if you plan on making more changes.

commit 59299f48cc57dea4c1bd5bf2f6dbefe925978080
Author: Akim Demaille <address@hidden>
Date:   Sun Jul 14 07:13:50 2019 +0200

    style: fixes in lalr1.cc
    
    * data/skeletons/stack.hh (stack::empty): New.
    * data/skeletons/lalr1.cc: No single-line if.
    Don't leave stray spaces.
    Prefer breaking lines before the operators.
    Space before parens.
    Prefer empty() to testing size().

diff --git a/data/skeletons/lalr1.cc b/data/skeletons/lalr1.cc
index c82a121d..60c2ba71 100644
--- a/data/skeletons/lalr1.cc
+++ b/data/skeletons/lalr1.cc
@@ -372,7 +372,7 @@ m4_define([b4_shared_declarations],
     mutable stack<by_state> yylac_stack_;
     /// Was an initial LAC context established?
     bool yy_lac_established_;
-    ]],[])[
+]])[
 
     /// Push a new state on the stack.
     /// \param m    a debug message to display
@@ -1142,9 +1142,9 @@ b4_dollar_popdef])[]dnl
     size_t lac_top = 0;
     while (true)
       {
-        state_type top_state = yylac_stack_.size () ?
-          yylac_stack_[0].state :
-          yystack_[lac_top].state;
+        state_type top_state = (yylac_stack_.empty ()
+                                ? yystack_[lac_top].state
+                                : yylac_stack_[0].state);
         int yyrule = yypact_[top_state];
         if (yy_pact_value_is_default_ (yyrule)
             || (yyrule += yytoken) < 0 || yylast_ < yyrule
@@ -1196,9 +1196,9 @@ b4_dollar_popdef])[]dnl
           lac_top += yylen;
         }
         // Keep top_state in sync with the updated stack.
-        top_state = yylac_stack_.size () ?
-           yylac_stack_[0].state :
-           yystack_[lac_top].state;
+        top_state = (yylac_stack_.empty ()
+                     ? yystack_[lac_top].state
+                     : yylac_stack_[0].state);
         // Push the resulting state of the reduction.
         state_type state = yy_lr_goto_state_ (top_state, yyr1_[yyrule]);
         YYCDEBUG << " G" << state;
@@ -1240,7 +1240,7 @@ b4_dollar_popdef])[]dnl
                  << yytname_[yytoken] << '\n';
 #endif
         yy_lac_established_ = true;
-        return yy_lac_check_(yytoken);
+        return yy_lac_check_ (yytoken);
       }
     return true;
   }
@@ -1320,7 +1320,8 @@ b4_error_verbose_if([state_type yystate, const 
symbol_type& yyla],
 #if ]b4_api_PREFIX[DEBUG
         // Execute LAC once. We don't care if it is succesful, we
         // only do it for the sake of debugging output.
-        if (!yy_lac_established_) yy_lac_check_(yytoken);
+        if (!yy_lac_established_)
+          yy_lac_check_ (yytoken);
 #endif]])[
 
         int yyn = yypact_[yystate];
diff --git a/data/skeletons/stack.hh b/data/skeletons/stack.hh
index 9fd2f9f0..f4e278e7 100644
--- a/data/skeletons/stack.hh
+++ b/data/skeletons/stack.hh
@@ -115,6 +115,13 @@ m4_define([b4_stack_define],
         return seq_.size ();
       }
 
+      /// Whether empty.
+      bool
+      empty () const
+      {
+        return seq_.empty ();
+      }
+
       /// Iterator on top of the stack (going downwards).
       const_iterator
       begin () const YY_NOEXCEPT