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

[Adrian Vogelsgesang]

Hi Akim,

Thanks for your detailed feedback and for the help with the test suite.
I took your branch `lac++` and rewrote the git history accordingly
to address your comments.
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.

Unfortunately, it seems that the Travis config is broken for pull-requests from
forks. The travis build is stuck in “Enter passphrase for /tmp/key.id_rsa:”.
But at least locally all tests including the new LAC C++ tests (except for 493 –
see below) are passing now with `--enable-gcc-warnings`.

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.

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.

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

> - 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 (`.

> - use 'while (true)', not 'while (1)'
Good point. I also prefer `true`. The `1` ended up there since I initially
copied the C code.

> - 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<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.
However, the test case uses ` AT_PURE_IF ([[*lvalp = 0;]])`. I guess
something is wrong with `AT_PURE_IF` here. I guess duplicating that
line with ` AT_CXX_IF` would fix the issue, but that seems bad style to
me. What should I do about it?

> - to avoid clashes between identifiers, prefer yytoken to token.
change the variable names from `token` to `yytoken`.

> - end comments with a period.
Done. I hope I found all of them.

> - 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`.

> - braces alone on their line (there is a "if (yy_lac_established_) {")
done

> - 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'm very afraid of your type changes in the code of yysyntax_error
> when !lac: you replaced int by yy_token_number
you are completely right. I reverted those changes. Thanks for the
detailed explanation.

Cheers,
Adrian

From: Akim Demaille <address@hidden>
Date: Saturday, 13 July 2019 at 09:18
To: Adrian Vogelsgesang <address@hidden>
Cc: "address@hidden" <address@hidden>
Subject: Re: Lookahead correction for the C++ skeleton lalr1.cc

Hi Adrian,

There's a number of issues you should address before we install
your contribution:

- please separate the part about yacc.c
- stay within 76 columns, at most 79 occasionally
- space before parens
- use 'while (true)', not 'while (1)'
- 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<https://travis-ci.org/akimd/bison/builds/558164975>
- to avoid clashes between identifiers, prefer yytoken to token.
- end comments with a period.
- there's a couple of useless "#if YYDEBUG" around a "YYCDEBUG"
- braces alone on their line (there is a "if (yy_lac_established_) {")
- please match the style of the git commit messages.
- I'm very afraid of your type changes in the code of yysyntax_error
when !lac: you replaced int by yy_token_number. I can understand
the desire to do it, but it's too dangerous. For instance if you
have 256 tokens, yy_token_number is unsigned char, but yyntokens_
is 256. So things like

yy_token_number yyxend = yychecklim < yyntokens_ ? yychecklim : yyntokens_;

are unlikely to behave properly.
I should probably add tests for 256 tokens in the test suite.
Because of hidden tokens such as eof, error, etc. the change
of type of yy_token_type is actually from 253 to 254 user tokens.
See script below.


I have prepared the test suite for your changes. Please see
the lac++ branch (on the official repo, and on GitHub too,
https://github.com/akimd/bison/tree/lac++)<https://github.com/akimd/bison/tree/lac++)>.

Cheers!



$ for i in {251..255}; do
cat >input.$i.y <<EOF
%token $(for j in {0..$i}; do printf " a$j"; done)
%%
exp:
EOF
~bison/_build/9d/tests/bison -LC++ input.$i.y -o input.$i.cc<http://i.cc> 
-Dparse.error=verbose
printf $i; grep 'token_number_type;' input.$i.cc<http://i.cc>
done

yacc-m4-readability.patch
Description: yacc-m4-readability.patch

cxx-lac-updated.patch
Description: cxx-lac-updated.patch