Re: [PATCH 0/3] yacc: compute the best type for the state number

[Akim Demaille]

Hi Paul,

Thanks for the quick response!

> Le 5 oct. 2019 à 10:48, Paul Eggert <address@hidden> a écrit :
> 
> On 10/3/19 10:02 PM, Akim Demaille wrote:
> 
>> I'm going back to Clang 3.3 and GCC 4.6.  That's all I managed to set up on 
>> Travis.  I would also like to try tcc, but only to run the tests, not build 
>> bison itself.  Travis also supports MSVC; when I feel brave enough, I'll see 
>> if I can use it for Bison.
> 
> For what it's worth, I can't build Bison master on Fedora 30 (the current 
> stable version of Fedora). Something to do with gettext version numbers.

I currently don't have Gettext 0.20 available on my distro, and bootstrap pulls 
gnulib-po which installs files from Gettext 0.20.  So I have to `cp 
po/Makefile.in.in gnulib-po`.  That's also what the CI does.


> At some point I suppose I should file a bug report. I work around the bug 
> with --disable-nls, but that's not compatible with --enable-gcc-warnings (so 
> I suppose I should file *two* bug reports :-).

I had not noticed this :(

> Also, I have been trying to build Bison master on Solaris 10 (the oldest 
> Solaris still supported) with Oracle Developer Studio 12.6 (the current 
> stable version). I found a few glitches and just now installed the attached. 
> I think there will be a few glitches more.

It's great that you tried that!  Nelson, at some point, had sent me logs about 
this platform, but that was a long time ago.

Maybe some of these fixes should make it into `maint`, in case we have to 
release 3.4.3.


>> We can introduce a %define variable to enable the new type for 
>> columns/locations, and hook it to %require 3.5 to promote the conversion.
> 
> Should we let the user specify the type (any integer type, I suppose), or 
> limit the user to just 'unsigned' and 'intmax_t'?

I'd rather play it safe and offer only a binary.  I am tired to writing tests 
for convoluted cases :)


> Perhaps call the type 'yy_pos_num' in yacc.c? (Is there a reason it's 
> yy_state_num in yacc.c but yyStateNum in glr.c?) Just thinking out loud.

Well, the reason is that yacc.c and the rest followed "our" style, but when 
Paul Hilfinger contributed glr.c, no-one told him to follow the GCS, and the 
code remained as is.  I would not object to migrate it to the "official" style, 
but I did not felt obligated to do so.  Even if that does mean that on 
occasions, for consistency between glr.c and yacc.c, I write 
snake_case_functions in glr.c, which ends up being self-inconsistent.


>>>   YY_CONVERT_INT_BEGIN
>>>   *yyssp = yystate;
>>>   YY_CONVERT_INT_END
>> Why would a warning here be bogus?  It looks to me like a perfect place to 
>> use a good old cast, as we're narrowing the type.
> 
> I don't like casts, because they mean the compiler won't catch serious errors 
> such as mistakenly assigning a pointer to an integer.

I fully agree.  Actually, C is too crude on casts: there's just one almighty 
cast.  C++ has split this in many casts (originally four, but there are more 
now, some of them being plain templated functions).  We could use macros to 
write clearer casts, say

        *yyssp = NARROWING_CAST (yy_state_num, yystate);

which could also, in debug mode, include bounds checking and magical pragmas to 
silence some compilers.  But one has 

But in this precise case, I don't see much of a difference between a cast and 
dark pragma magic: in both cases it means "dear compiler, shut up", yet the 
cast is truly portable.


> Other GNU programs typically do not enable -Wconversion or 
> -Wimplicit-int-conversion because they generate too many false alarms. I 
> figured I could remove the need for casts by disabling the misguided warnings.
> 
> However, after seeing your diagnostics I suppose that the pragmas are more 
> trouble than they're worth. Bison skeletons should be robust even in the 
> presence of misguided compiler options like -Wconversion (because some apps 
> are foolish enough to use such options :-) and it's such a pain to turn off 
> these options portably that casts are probably the way to go in skeletons, 
> despite their flaws.

Right.


>> GCC 8 with ASAN (https://api.travis-ci.org/v3/job/592926526/log.txt):
> 
> That URL uses Clang 8, not GCC 8. Of course Bison should work with Clang too, 
> but see below.
>>> 113. output.at:293: testing Output file name: `~!@#$%^&*()-=_+{}[]|\:;<>, 
>>> .' ...
>>> ../../tests/output.at:293: touch "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.tmp" || 
>>> exit 77
>>> ../../tests/output.at:293: COLUMNS=1000; export COLUMNS;  bison --color=no 
>>> -fno-caret -o "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.c" 
>>> --defines="\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.h" glr.y
>>> ../../tests/output.at:293: ls "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.c" 
>>> "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.h"
>>> stdout:
>>> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.c
>>> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.h
>>> ../../tests/output.at:293: $CC $CFLAGS $CPPFLAGS  -c -o glr.o -c 
>>> "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.c"
>>> stderr:
>>> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.c:1467:37: error: operand of ? changes 
>>> signedness: 'ptrdiff_t' (aka 'long') to 'unsigned long' 
>>> [-Werror,-Wsign-conversion]
>>>       ptrdiff_t half_max_capacity = YYSIZEMAX / (2 * sizeof yynewStates[0]);
>>>                                     ^~~~~~~~~ ~
>>> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.c:132:59: note: expanded from macro 
>>> 'YYSIZEMAX'
>>> #define YYSIZEMAX (PTRDIFF_MAX < SIZE_MAX ? PTRDIFF_MAX : (ptrdiff_t) 
>>> SIZE_MAX)
> 
> This is a bug in Clang, as both operands of ? are ptrdiff_t. You might try 
> filing a bug report with the Clang folks.

I'll do that.  Yet we need to find a way to have this be silenced.  I tried 
also casting PTRDIFF_MAX to (ptrdiff_t), to no avail.  Maybe the error in 
actually in the division by the unsigned and the diagnostic is wrong.  I'll try 
that.  Can't reproduce locally, it's on the CI only.

> My experience with Clang warnings has not been great. Clang issues multiple 
> bogus warnings when compiling Bison itself, and there's little point to 
> enabling warnings, particularly in the test cases since Clang issues a bunch 
> of false alarms, so many that it's not worth looking for wheat among the 
> chaff. I suggest using --enable-gcc-warnings only with recent versions of 
> GCC, which is my practice in other projects. It's too much work to pacify 
> Clang or older GCC versions, and there's little benefit to doing so.

Actually configure's --enable-gcc-warnings is also what selects the warnings 
for the test suite, for which I really want to have no warnings (because that's 
fewer complaints).


Cheers!