GCC 11

[Saulius Krasuckas]

Hello folks,

* On 2021-09-29 12:17, Eric Pouech wrote:
> For sharing :
> 
> - newest gcc version (11) sets by default of bunch of new tests for 
> code correctness
> - it spits out around ten thousand lines of warnings (*), making it 
> quite impossible to look into compiler output

Eric, thanks for investigating this.
I have stopped actively contributing for over 15y already, but I still 
would like to express my opinion.

> The false positives we need to tackle:
> 
> A) misleading indentation
> ===================
> GCC11 complains about code like:
>         foo;
> todo_wine
>         ok(tst, ...);
>         bar;
> 
> (gcc11 warns about 'bar ' not being properly indented wrt todo_wine)

I wonder whether/how it differentiates between spaces and tabs here.

> there are (as of today) 817 occurrences of those in wine tree (spread 
> across 127 files)
> and counts for ~90% (*) of the line of warnings generated by GCC
> only 2 true positives are generated by this GCC warning
> 
> Remarks:
> 
> - constructs on a single line don't generate the warning
>      todo_wine ok(tst, ...); // ok no warning, whatever the indentation 
> of the line wrt the rest of the code
> - having the ok(tst, ...) inside a block after the todo_wine doesn't 
> generate the warning

IMO, it would be nice to have a way to achieve a single indentation 
style for all todo_wine* names.

Has the community decided on a single opinion (had it any polls) in that 
regard already?  That would help.

I remember seemingly similar equivocal state of mixing tabs and spaces 
in indentations present during 2003-2006.
Has something changed in that regard by a chance?

> possible solutions:
> 
> - disable the warning in configure
>   cons: will not trigger on true positive
>   pros: minor impact on code base (either committed and under progress 
> in dev:s trees)
> 
> - merge the todo_wine and ok() line into a single one
>   pros: keep current indentation, coherent with todo_wine + block
>   cons: harder to read, esp. for todo_wine_if
>   cons: for todo_wine_if, could generate too long lines
>   cons: large impact on code base
>   pros/cons: git blame on ok() line will tell who removed the 
> todo_wine, not who wrote the test

The last bullet is definitely a contra to me: git blame should not 
mention a person who hasn't changed the ok() logic and just removed the 
todo_wine*.  And changing the logic should not be reflected on any code 
line containing a todo_wine*.  That way it's a lot clearer who did what 
with that line in the past.

> - reindent only problematic todo_wine
>         foo;
> todo_wine
>         ok(tst, ...);
>         bar;
>     ^ without indenting the line ok(tst, ...);
>     ^ without indenting the "todo_wine {\n<...>\n}" nor the "todo_wine 
> ok()" on a single line constructs as they don't generate warnings
> 
>    pros/cons: indentation isn't used for adding comments to the code 
> (that's what comments are for)
>    cons: large impact on code base

My vote goes for this (if no better decisions are found).

I still would like to be able to have todo_wine* in beginning of a line 
as it introduces less visual noise into reading the original logic of 
test (for me).  Plus, it acts as a sharp reminder to fix the test every 
time I read it. :)

> - reindent all todo_wine
>    same as above, but also reindenting the "todo_wine {\n<...>\n}" and 
> the "todo_wine ok()" on a single line constructs
>    pros/cons: indentation isn't used for adding comments to the code 
> (that's what comments are for)
>    cons: large impact on code base

Now as I read source [*], I think it gets down to the macro embedding a 
for() loop:

  128 #define todo_if(is_todo) for (winetest_start_todo(is_todo); \
  129                               winetest_loop_todo(); \
  130                               winetest_end_todo())

Umm, would adding backslash on the line #130 too help to avoid the 
warning?  No idea since I am away from the compiler/development box.

S.

[*] 
https://source.winehq.org/git/wine.git/blob/HEAD:/include/wine/test.h#l128