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
there are a couple of genuine issues reported by gcc11
- Rémi and I already sent a serie of patches for addressing those
- a strange indentation waiting for a volunteer
(in dlls/wininet/internet.c:2878, the line lpwhh->ErrorMask = *(ULONG*)lpBuffer;
at the end of the if/else chain is misindented)
unfortunately, most of the generated warnings are false positives, that need to be fixed (or removerd) before GCC11 becomes mainstream
- Fedora 35 with GCC11 is in beta right now, and GA is mid of October
- I haven't checked other distros though, but I expect similar evolution in coming weeks/months
- in Fedora both gcc and mingw-gcc have been upgraded to version 11
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)
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
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
- 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
- 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
B) uninitialized objects
===============
GCC11 warns on code like:
void foo(const TYPE* s);
.....
TYPE s;
foo(&s);
because, s isn't initialized, and is passed as const ptr, hence could be read by function foo without being initialized
proposal:
- depending on context, different fixes are at hand: either initialize s, or when applicable, play with aliasing
C) partial structures
============
In a bunch of places, we use a pointer to a structure, where underlying allocated object is smaller than the actual size of the structure.
(roughly 400 lines of warnings (*))
It's used:
- when dealing with two structures sharing their first fields (like in BITMAP for example)
- in tests, to test behavior will various sizes of objects as input...
proposal: disable this warning in configure
D) sizeof/sizeof
=========
GCC11 warns when sizeof(foo)/sizeof(bar) is used and cannot be reduced to computing the number of elements of an array
proposal: rewrite as sizeof(foo)/(sizeof(bar)) (note the parenthesis around sizeof(bar), as suggested by GCC)
A+
(*) those values are rough estimates, only consider the order of magnitude
The logfile from the WineHQ Ubuntu 21.04 build on OBS is here: [ https://build.opensuse.org/build/Emulators:Wine:Debian/xUbuntu_21.04/x86_64/... | https://build.opensuse.org/build/Emulators:Wine:Debian/xUbuntu_21.04/x86_64/... ]
I could not immediately find warnings about what you are talking about here, but there sure is enough warnings about "format argument" (-Wformat and -Wstrict-aliasing) stuff, that i don't get when i compile wine myself locally. Might not be related to what you ask here, but since you were talking about "gcc 11 becomes mainstream", i thought it was worth to point to the official OBS buildlog for Ubuntu 21.04 - that uses gcc-11 (although gcc-mingw-w64 is still at 10.3).
I guess debian/ubuntu "debuild" system sets these -Wformat= and -Wstrict-aliasing options when building as default, and that seem to throw a lot of warnings. I did not browse through the entire log, but maybe it is just some overly eager checks that some distro's enable by default, that it is not really critical?
Do you have a "sample warning" for me to search for? Eg. dlls/ncrypt/main.c:106:11: warning: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'NCRYPT_PROV_HANDLE' {aka 'long long unsigned int'} [-Wformat=]
Sveinar
----- On Sep 29, 2021, at 11:17 AM, Eric Pouech eric.pouech@orange.fr 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
there are a couple of genuine issues reported by gcc11
- Rémi and I already sent a serie of patches for addressing those
- a strange indentation waiting for a volunteer
(in dlls/ wininet/internet.c:2878, the line lpwhh->ErrorMask = *(ULONG*)lpBuffer;
at the end of the if/else chain is misindented)
unfortunately, most of the generated warnings are false positives, that need to be fixed (or removerd) before GCC11 becomes mainstream
- Fedora 35 with GCC11 is in beta right now, and GA is mid of October
- I haven't checked other distros though, but I expect similar evolution in
coming weeks/months
- in Fedora both gcc and mingw-gcc have been upgraded to version 11
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)
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
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
- 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
- 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
B) uninitialized objects
===============
GCC11 warns on code like:
void foo(const TYPE* s);
.....
TYPE s;
foo(&s);
because, s isn't initialized, and is passed as const ptr, hence could be read by function foo without being initialized
proposal:
- depending on context, different fixes are at hand: either initialize s, or
when applicable, play with aliasing
C) partial structures
============
In a bunch of places, we use a pointer to a structure, where underlying allocated object is smaller than the actual size of the structure.
(roughly 400 lines of warnings (*))
It's used:
- when dealing with two structures sharing their first fields (like in BITMAP
for example)
- in tests, to test behavior will various sizes of objects as input...
proposal: disable this warning in configure
D) sizeof/sizeof
=========
GCC11 warns when sizeof(foo)/sizeof(bar) is used and cannot be reduced to computing the number of elements of an array
proposal: rewrite as sizeof(foo)/(sizeof(bar)) (note the parenthesis around sizeof(bar), as suggested by GCC)
A+
(*) those values are rough estimates, only consider the order of magnitude
Le 29/09/2021 à 13:41, Sveinar Søpler a écrit :
The logfile from the WineHQ Ubuntu 21.04 build on OBS is here: https://build.opensuse.org/build/Emulators:Wine:Debian/xUbuntu_21.04/x86_64/...
I could not immediately find warnings about what you are talking about here, but there sure is enough warnings about "format argument" (-Wformat and -Wstrict-aliasing) stuff, that i don't get when i compile wine myself locally. Might not be related to what you ask here, but since you were talking about "gcc 11 becomes mainstream", i thought it was worth to point to the official OBS buildlog for Ubuntu 21.04 - that uses gcc-11 (although gcc-mingw-w64 is still at 10.3).
I guess debian/ubuntu "debuild" system sets these -Wformat= and -Wstrict-aliasing options when building as default, and that seem to throw a lot of warnings. I did not browse through the entire log, but maybe it is just some overly eager checks that some distro's enable by default, that it is not really critical?
Do you have a "sample warning" for me to search for? Eg. dlls/ncrypt/main.c:106:11: warning: format '%lx' expects argument of type 'long unsigned int', but argument 5 has type 'NCRYPT_PROV_HANDLE' {aka 'long long unsigned int'} [-Wformat=]
to correct myself:
- I thought the trigger was "simply" the upgrade to GCC version 11... after some quick search, Fedora claims moving from 11.0 (F34) to 11.2 (F35), but looks like F34 is pushing gcc 11.2 in updates since July... so the root cause of those new warnings need to be clarified
- the warnings I'm seeing are triggered by -Wmisleading-indentation, -Warray-bounds, -Wsizeof-array-div, -Wmaybe-uninitialized, which are all active when -Wall is used
it's strange from the log link above that non of the options work with the cross compiler
[ 569s] checking whether the cross-compiler supports -target x86_64-w64-mingw32 -fuse-ld=lld... no [ 569s] checking whether the cross-compiler supports -fno-strict-aliasing... no [ 569s] checking whether the cross-compiler supports -Werror=unknown-warning-option... no [ 569s] checking whether the cross-compiler supports -Werror=ignored-optimization-argument... no [ 569s] checking whether the cross-compiler supports -Wdeclaration-after-statement... no [ 570s] checking whether the cross-compiler supports -Wempty-body... no [ 570s] checking whether the cross-compiler supports -Wignored-qualifiers... no [ 570s] checking whether the cross-compiler supports -Winit-self... no [ 570s] checking whether the cross-compiler supports -Wpacked-not-aligned... no [ 570s] checking whether the cross-compiler supports -Wpragma-pack... no [ 570s] checking whether the cross-compiler supports -Wshift-overflow=2... no [ 570s] checking whether the cross-compiler supports -Wstrict-prototypes... no [ 570s] checking whether the cross-compiler supports -Wtype-limits... no [ 570s] checking whether the cross-compiler supports -Wunused-but-set-parameter... no [ 570s] checking whether the cross-compiler supports -Wvla... no [ 570s] checking whether the cross-compiler supports -Wwrite-strings... no [ 570s] checking whether the cross-compiler supports -Wpointer-arith... no [ 570s] checking whether the cross-compiler supports -Wlogical-op... no [ 570s] checking whether the cross-compiler supports -Wabsolute-value... no [ 570s] checking whether the cross-compiler supports -Wno-format... no [ 570s] checking whether the cross-compiler supports -Wformat-overflow... no [ 570s] checking whether the cross-compiler supports -Wnonnull... no [ 570s] checking whether the cross-compiler supports -mcx16... no [ 570s] checking whether the cross-compiler supports -gdwarf-2... no [ 571s] checking whether the cross-compiler supports -gstrict-dwarf... no [ 571s] checking whether the cross-compiler supports -fexcess-precision=standard... no I'd expect that a couple of them are supported
Sveinar
On 29.09.2021 16:42, Eric Pouech wrote:
to correct myself:
- I thought the trigger was "simply" the upgrade to GCC version 11...
after some quick search, Fedora claims moving from 11.0 (F34) to 11.2 (F35), but looks like F34 is pushing gcc 11.2 in updates since July... so the root cause of those new warnings need to be clarified
- the warnings I'm seeing are triggered by -Wmisleading-indentation,
-Warray-bounds, -Wsizeof-array-div, -Wmaybe-uninitialized, which are all active when -Wall is used
it's strange from the log link above that non of the options work with the cross compiler
[ 569s] checking whether the cross-compiler supports -target x86_64-w64-mingw32 -fuse-ld=lld... no [ 569s] checking whether the cross-compiler supports -fno-strict-aliasing... no [ 569s] checking whether the cross-compiler supports -Werror=unknown-warning-option... no [ 569s] checking whether the cross-compiler supports -Werror=ignored-optimization-argument... no [ 569s] checking whether the cross-compiler supports -Wdeclaration-after-statement... no [ 570s] checking whether the cross-compiler supports -Wempty-body... no [ 570s] checking whether the cross-compiler supports -Wignored-qualifiers... no [ 570s] checking whether the cross-compiler supports -Winit-self... no [ 570s] checking whether the cross-compiler supports -Wpacked-not-aligned... no [ 570s] checking whether the cross-compiler supports -Wpragma-pack... no [ 570s] checking whether the cross-compiler supports -Wshift-overflow=2... no [ 570s] checking whether the cross-compiler supports -Wstrict-prototypes... no [ 570s] checking whether the cross-compiler supports -Wtype-limits... no [ 570s] checking whether the cross-compiler supports -Wunused-but-set-parameter... no [ 570s] checking whether the cross-compiler supports -Wvla... no [ 570s] checking whether the cross-compiler supports -Wwrite-strings... no [ 570s] checking whether the cross-compiler supports -Wpointer-arith... no [ 570s] checking whether the cross-compiler supports -Wlogical-op... no [ 570s] checking whether the cross-compiler supports -Wabsolute-value... no [ 570s] checking whether the cross-compiler supports -Wno-format... no [ 570s] checking whether the cross-compiler supports -Wformat-overflow... no [ 570s] checking whether the cross-compiler supports -Wnonnull... no [ 570s] checking whether the cross-compiler supports -mcx16... no [ 570s] checking whether the cross-compiler supports -gdwarf-2... no [ 571s] checking whether the cross-compiler supports -gstrict-dwarf... no [ 571s] checking whether the cross-compiler supports -fexcess-precision=standard... no I'd expect that a couple of them are supported
That was indeed a bit strange. When i do just a regular configure --enable-win64 without any flags, i get this on Ubuntu 21.04:
checking for x86_64-w64-mingw32-gcc... x86_64-w64-mingw32-gcc checking whether x86_64-w64-mingw32-gcc works... yes checking whether the cross-compiler supports -target x86_64-w64-mingw32 -fuse-ld=lld... no checking whether the cross-compiler supports -fno-strict-aliasing... yes checking whether the cross-compiler supports -Werror=unknown-warning-option... no checking whether the cross-compiler supports -Werror=ignored-optimization-argument... no checking whether the cross-compiler supports -Wdeclaration-after-statement... yes checking whether the cross-compiler supports -Wempty-body... yes checking whether the cross-compiler supports -Wignored-qualifiers... yes checking whether the cross-compiler supports -Winit-self... yes checking whether the cross-compiler supports -Wpacked-not-aligned... yes checking whether the cross-compiler supports -Wpragma-pack... no checking whether the cross-compiler supports -Wshift-overflow=2... yes checking whether the cross-compiler supports -Wstrict-prototypes... yes checking whether the cross-compiler supports -Wtype-limits... yes checking whether the cross-compiler supports -Wunused-but-set-parameter... yes checking whether the cross-compiler supports -Wvla... yes checking whether the cross-compiler supports -Wwrite-strings... yes checking whether the cross-compiler supports -Wpointer-arith... yes checking whether the cross-compiler supports -Wlogical-op... yes checking whether the cross-compiler supports -Wabsolute-value... yes checking whether the cross-compiler supports -Wno-format... yes checking whether the cross-compiler supports -Wformat-overflow... yes checking whether the cross-compiler supports -Wnonnull... yes checking whether the cross-compiler supports -mcx16... yes checking whether the cross-compiler supports -gdwarf-2... yes checking whether the cross-compiler supports -gstrict-dwarf... yes checking whether the cross-compiler supports -fexcess-precision=standard... yes
I do kinda fear this pulls us down a unrelated rabbithole - some buildflags or "OBS quirks" perhaps.. dunno. In short: The log from WineHQ buildserver does not really look like it should :)
Sveinar
Well, debuild seem to put the same CFLAGS/LDFLAGS to gcc-mingw-w64 as it does to gcc, and i guess one of the "debian hardening" options puts some flags to mingw's LD that does not fly. This does not happen when you build "manually", so it is quite likely just a build config thing.
Sorry, for this noise, as this is most certainly unrelated to the original post. I will do some investigations towards debian build settings, but i do not think it has much to do with the gcc-11 concerns you have i am afraid, although it could be worth investigating "build system flags vs. manual compile" (atleast i find it interesting).
Again, i am sorry for the noise.
Sveinar
On 29.09.2021 18:18, Sveinar Søpler wrote:
Sveinar
On 29.09.2021 16:42, Eric Pouech wrote:
to correct myself:
- I thought the trigger was "simply" the upgrade to GCC version 11...
after some quick search, Fedora claims moving from 11.0 (F34) to 11.2 (F35), but looks like F34 is pushing gcc 11.2 in updates since July... so the root cause of those new warnings need to be clarified
- the warnings I'm seeing are triggered by -Wmisleading-indentation,
-Warray-bounds, -Wsizeof-array-div, -Wmaybe-uninitialized, which are all active when -Wall is used
it's strange from the log link above that non of the options work with the cross compiler
[ 569s] checking whether the cross-compiler supports -target x86_64-w64-mingw32 -fuse-ld=lld... no [ 569s] checking whether the cross-compiler supports -fno-strict-aliasing... no [ 569s] checking whether the cross-compiler supports -Werror=unknown-warning-option... no [ 569s] checking whether the cross-compiler supports -Werror=ignored-optimization-argument... no [ 569s] checking whether the cross-compiler supports -Wdeclaration-after-statement... no [ 570s] checking whether the cross-compiler supports -Wempty-body... no [ 570s] checking whether the cross-compiler supports -Wignored-qualifiers... no [ 570s] checking whether the cross-compiler supports -Winit-self... no [ 570s] checking whether the cross-compiler supports -Wpacked-not-aligned... no [ 570s] checking whether the cross-compiler supports -Wpragma-pack... no [ 570s] checking whether the cross-compiler supports -Wshift-overflow=2... no [ 570s] checking whether the cross-compiler supports -Wstrict-prototypes... no [ 570s] checking whether the cross-compiler supports -Wtype-limits... no [ 570s] checking whether the cross-compiler supports -Wunused-but-set-parameter... no [ 570s] checking whether the cross-compiler supports -Wvla... no [ 570s] checking whether the cross-compiler supports -Wwrite-strings... no [ 570s] checking whether the cross-compiler supports -Wpointer-arith... no [ 570s] checking whether the cross-compiler supports -Wlogical-op... no [ 570s] checking whether the cross-compiler supports -Wabsolute-value... no [ 570s] checking whether the cross-compiler supports -Wno-format... no [ 570s] checking whether the cross-compiler supports -Wformat-overflow... no [ 570s] checking whether the cross-compiler supports -Wnonnull... no [ 570s] checking whether the cross-compiler supports -mcx16... no [ 570s] checking whether the cross-compiler supports -gdwarf-2... no [ 571s] checking whether the cross-compiler supports -gstrict-dwarf... no [ 571s] checking whether the cross-compiler supports -fexcess-precision=standard... no I'd expect that a couple of them are supported
That was indeed a bit strange. When i do just a regular configure --enable-win64 without any flags, i get this on Ubuntu 21.04:
checking for x86_64-w64-mingw32-gcc... x86_64-w64-mingw32-gcc checking whether x86_64-w64-mingw32-gcc works... yes checking whether the cross-compiler supports -target x86_64-w64-mingw32 -fuse-ld=lld... no checking whether the cross-compiler supports -fno-strict-aliasing... yes checking whether the cross-compiler supports -Werror=unknown-warning-option... no checking whether the cross-compiler supports -Werror=ignored-optimization-argument... no checking whether the cross-compiler supports -Wdeclaration-after-statement... yes checking whether the cross-compiler supports -Wempty-body... yes checking whether the cross-compiler supports -Wignored-qualifiers... yes checking whether the cross-compiler supports -Winit-self... yes checking whether the cross-compiler supports -Wpacked-not-aligned... yes checking whether the cross-compiler supports -Wpragma-pack... no checking whether the cross-compiler supports -Wshift-overflow=2... yes checking whether the cross-compiler supports -Wstrict-prototypes... yes checking whether the cross-compiler supports -Wtype-limits... yes checking whether the cross-compiler supports -Wunused-but-set-parameter... yes checking whether the cross-compiler supports -Wvla... yes checking whether the cross-compiler supports -Wwrite-strings... yes checking whether the cross-compiler supports -Wpointer-arith... yes checking whether the cross-compiler supports -Wlogical-op... yes checking whether the cross-compiler supports -Wabsolute-value... yes checking whether the cross-compiler supports -Wno-format... yes checking whether the cross-compiler supports -Wformat-overflow... yes checking whether the cross-compiler supports -Wnonnull... yes checking whether the cross-compiler supports -mcx16... yes checking whether the cross-compiler supports -gdwarf-2... yes checking whether the cross-compiler supports -gstrict-dwarf... yes checking whether the cross-compiler supports -fexcess-precision=standard... yes
I do kinda fear this pulls us down a unrelated rabbithole - some buildflags or "OBS quirks" perhaps.. dunno. In short: The log from WineHQ buildserver does not really look like it should :)
Sveinar
Le 29/09/2021 à 21:44, Sveinar Søpler a écrit :
Well, debuild seem to put the same CFLAGS/LDFLAGS to gcc-mingw-w64 as it does to gcc, and i guess one of the "debian hardening" options puts some flags to mingw's LD that does not fly. This does not happen when you build "manually", so it is quite likely just a build config thing.
Sorry, for this noise, as this is most certainly unrelated to the original post. I will do some investigations towards debian build settings, but i do not think it has much to do with the gcc-11 concerns you have i am afraid, although it could be worth investigating "build system flags vs. manual compile" (atleast i find it interesting).
Again, i am sorry for the noise.
yes, looks more something like that
after some more investigations, it's quite clear that most of the warnings comes with gcc 11.1 and even more with gcc 11.2.
so it should be replaced the title (and content) of this thread gcc11 by gcc11.2
A+
Sveinar
On 30.09.2021 12:03, Eric Pouech wrote:
Le 29/09/2021 à 21:44, Sveinar Søpler a écrit :
Well, debuild seem to put the same CFLAGS/LDFLAGS to gcc-mingw-w64 as it does to gcc, and i guess one of the "debian hardening" options puts some flags to mingw's LD that does not fly. This does not happen when you build "manually", so it is quite likely just a build config thing.
Sorry, for this noise, as this is most certainly unrelated to the original post. I will do some investigations towards debian build settings, but i do not think it has much to do with the gcc-11 concerns you have i am afraid, although it could be worth investigating "build system flags vs. manual compile" (atleast i find it interesting).
Again, i am sorry for the noise.
yes, looks more something like that
after some more investigations, it's quite clear that most of the warnings comes with gcc 11.1 and even more with gcc 11.2.
so it should be replaced the title (and content) of this thread gcc11 by gcc11.2
I am not able to reproduce these warnings with GCC-11.1 on Ubuntu 21.04 when building with debuild locally. Attaching build.log from 64 bit build and config.log from configure.
The issues on the OBS buildserver with all the warnings was fixed by removing -Wl,-z,relro (a "hardening" set flag) from the buildsettings.
A+
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
Le 02/10/2021 à 15:51, Saulius Krasuckas a écrit :
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.
doc says that that tabs are expanded by default to 8 characters (can be overriden with tab-stop option)
so much space for playing 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.
no, different authors => different styles
(note that there other culprits as if (0) (but there are others too) that have to be taken into account
- 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.
no the problem doesn't stem from the line after the todo, but the second line after the todo (the bar one above) which gcc doesn't like as not being indented as the todo
thanks for jumping in
A+
-
Eric Pouech -
Saulius Krasuckas -
Sveinar Søpler