| Message ID | 1446137381-32748-5-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/29/2015 10:49 AM, David Malcolm wrote: > Our documentation describes -Wall as enabling "all the warnings about > constructions that some users consider questionable, and that are easy to avoid > (or modify to prevent the warning), even in conjunction with macros." > > I believe that -Wmisleading-indentation meets these criteria, and is > likely to be of benefit to users who may not read release notes; it > warns for indentation that's misleading, but not for indentation > that's merely bad: the former are places where a user will likely > want to fix the code. > > The fix is usually easy and obvious: fix the misleadingly-indented > code. If that isn't an option for some reason, pragmas can be used to > turn off the warning for a particular fragment of code: > > #pragma GCC diagnostic push > #pragma GCC diagnostic ignored "-Wmisleading-indentation" > if (flag) > x = 3; > y = 2; > #pragma GCC diagnostic pop > > -Wmisleading-indentation has been tested with a variety of indentation > styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c) > and on a variety of real-world projects. For example, in: > https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html > Patrick reports: > "Tested by building the linux, git, vim, sqlite and gdb-binutils sources > with -Wmisleading-indentation." > > With the tweak earlier in this kit I believe we now have a good > enough signal:noise ratio for this warning to be widely used; hence this > patch adds the warning to -Wall. > > Bootstrapped®rtested with x86_64-pc-linux-gnu. > > OK for trunk? > > gcc/c-family/ChangeLog: > * c.opt (Wmisleading-indentation): Add to -Wall for C and C++. > > gcc/ChangeLog: > * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the > list. > (-Wmisleading-indentation): Update documentation to reflect > being enabled by -Wall in C/C++. I'm sure we'll get some grief for this :-) Approved once we're clean in GCC. I'm going to explicitly say that we'll have to watch for fallout, particularly as we start getting feedback from Debian & Fedora mass-rebuilds as we approach release time. If the fallout is too bad, we'll have to reconsider. I'll pre-approve patches which fix anything caught by this option in GCC as long as the fix just adjusts whitespace :-) jeff
On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote: > On 10/29/2015 10:49 AM, David Malcolm wrote: >> >> Our documentation describes -Wall as enabling "all the warnings about >> constructions that some users consider questionable, and that are easy to >> avoid >> (or modify to prevent the warning), even in conjunction with macros." >> >> I believe that -Wmisleading-indentation meets these criteria, and is >> likely to be of benefit to users who may not read release notes; it >> warns for indentation that's misleading, but not for indentation >> that's merely bad: the former are places where a user will likely >> want to fix the code. >> >> The fix is usually easy and obvious: fix the misleadingly-indented >> code. If that isn't an option for some reason, pragmas can be used to >> turn off the warning for a particular fragment of code: >> >> #pragma GCC diagnostic push >> #pragma GCC diagnostic ignored "-Wmisleading-indentation" >> if (flag) >> x = 3; >> y = 2; >> #pragma GCC diagnostic pop >> >> -Wmisleading-indentation has been tested with a variety of indentation >> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c) >> and on a variety of real-world projects. For example, in: >> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html >> Patrick reports: >> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources >> with -Wmisleading-indentation." >> >> With the tweak earlier in this kit I believe we now have a good >> enough signal:noise ratio for this warning to be widely used; hence this >> patch adds the warning to -Wall. >> >> Bootstrapped®rtested with x86_64-pc-linux-gnu. >> >> OK for trunk? >> >> gcc/c-family/ChangeLog: >> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++. >> >> gcc/ChangeLog: >> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the >> list. >> (-Wmisleading-indentation): Update documentation to reflect >> being enabled by -Wall in C/C++. > > I'm sure we'll get some grief for this :-) > > Approved once we're clean in GCC. I'm going to explicitly say that we'll > have to watch for fallout, particularly as we start getting feedback from > Debian & Fedora mass-rebuilds as we approach release time. If the fallout > is too bad, we'll have to reconsider. > > I'll pre-approve patches which fix anything caught by this option in GCC as > long as the fix just adjusts whitespace :-) Please at least check also binutils and gdb and other packages that use -Werror (well, just rebuild Fedora world). I'd say this shouldn't be in -Wall ... (and I suppose I'll happily patch it out of SUSE GCC ...). Maybe put it into -Wextra? Richard. > jeff > >
On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote: >> On 10/29/2015 10:49 AM, David Malcolm wrote: >>> >>> Our documentation describes -Wall as enabling "all the warnings about >>> constructions that some users consider questionable, and that are easy to >>> avoid >>> (or modify to prevent the warning), even in conjunction with macros." >>> >>> I believe that -Wmisleading-indentation meets these criteria, and is >>> likely to be of benefit to users who may not read release notes; it >>> warns for indentation that's misleading, but not for indentation >>> that's merely bad: the former are places where a user will likely >>> want to fix the code. >>> >>> The fix is usually easy and obvious: fix the misleadingly-indented >>> code. If that isn't an option for some reason, pragmas can be used to >>> turn off the warning for a particular fragment of code: >>> >>> #pragma GCC diagnostic push >>> #pragma GCC diagnostic ignored "-Wmisleading-indentation" >>> if (flag) >>> x = 3; >>> y = 2; >>> #pragma GCC diagnostic pop >>> >>> -Wmisleading-indentation has been tested with a variety of indentation >>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c) >>> and on a variety of real-world projects. For example, in: >>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html >>> Patrick reports: >>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources >>> with -Wmisleading-indentation." >>> >>> With the tweak earlier in this kit I believe we now have a good >>> enough signal:noise ratio for this warning to be widely used; hence this >>> patch adds the warning to -Wall. >>> >>> Bootstrapped®rtested with x86_64-pc-linux-gnu. >>> >>> OK for trunk? >>> >>> gcc/c-family/ChangeLog: >>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++. >>> >>> gcc/ChangeLog: >>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the >>> list. >>> (-Wmisleading-indentation): Update documentation to reflect >>> being enabled by -Wall in C/C++. >> >> I'm sure we'll get some grief for this :-) >> >> Approved once we're clean in GCC. I'm going to explicitly say that we'll >> have to watch for fallout, particularly as we start getting feedback from >> Debian & Fedora mass-rebuilds as we approach release time. If the fallout >> is too bad, we'll have to reconsider. >> >> I'll pre-approve patches which fix anything caught by this option in GCC as >> long as the fix just adjusts whitespace :-) > > Please at least check also binutils and gdb and other packages that use -Werror > (well, just rebuild Fedora world). FYI binutils, gdb and glibc, from git, all fail to build due to -Wmisleading-indentation warnings/errors. (None of the warnings are bogus (IMO), though I don't think any of the warnings expose a real bug either.)
On Sun, 2015-11-01 at 17:06 -0500, Patrick Palka wrote: > On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener > <richard.guenther@gmail.com> wrote: > > On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote: > >> On 10/29/2015 10:49 AM, David Malcolm wrote: > >>> > >>> Our documentation describes -Wall as enabling "all the warnings about > >>> constructions that some users consider questionable, and that are easy to > >>> avoid > >>> (or modify to prevent the warning), even in conjunction with macros." > >>> > >>> I believe that -Wmisleading-indentation meets these criteria, and is > >>> likely to be of benefit to users who may not read release notes; it > >>> warns for indentation that's misleading, but not for indentation > >>> that's merely bad: the former are places where a user will likely > >>> want to fix the code. > >>> > >>> The fix is usually easy and obvious: fix the misleadingly-indented > >>> code. If that isn't an option for some reason, pragmas can be used to > >>> turn off the warning for a particular fragment of code: > >>> > >>> #pragma GCC diagnostic push > >>> #pragma GCC diagnostic ignored "-Wmisleading-indentation" > >>> if (flag) > >>> x = 3; > >>> y = 2; > >>> #pragma GCC diagnostic pop > >>> > >>> -Wmisleading-indentation has been tested with a variety of indentation > >>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c) > >>> and on a variety of real-world projects. For example, in: > >>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html > >>> Patrick reports: > >>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources > >>> with -Wmisleading-indentation." > >>> > >>> With the tweak earlier in this kit I believe we now have a good > >>> enough signal:noise ratio for this warning to be widely used; hence this > >>> patch adds the warning to -Wall. > >>> > >>> Bootstrapped®rtested with x86_64-pc-linux-gnu. > >>> > >>> OK for trunk? > >>> > >>> gcc/c-family/ChangeLog: > >>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++. > >>> > >>> gcc/ChangeLog: > >>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the > >>> list. > >>> (-Wmisleading-indentation): Update documentation to reflect > >>> being enabled by -Wall in C/C++. > >> > >> I'm sure we'll get some grief for this :-) > >> > >> Approved once we're clean in GCC. I'm going to explicitly say that we'll > >> have to watch for fallout, particularly as we start getting feedback from > >> Debian & Fedora mass-rebuilds as we approach release time. If the fallout > >> is too bad, we'll have to reconsider. > >> > >> I'll pre-approve patches which fix anything caught by this option in GCC as > >> long as the fix just adjusts whitespace :-) > > > > Please at least check also binutils and gdb and other packages that use -Werror > > (well, just rebuild Fedora world). > > FYI binutils, gdb and glibc, from git, all fail to build due to > -Wmisleading-indentation warnings/errors. (None of the warnings are > bogus (IMO), though I don't think any of the warnings expose a real > bug either.) Bother. Do you happen to have the logs handy? (or could you upload the somewhere?) I tried building binutils+gdb 854eb72b00ba46d65ce36dc3432f01e223ce44cb with gcc6+this kit (on x86_64) but I get: In file included from ../../src/bfd/archive.c:143:0: ../../src/bfd/../include/bfdlink.h:452:38: error: field ‘compress_debug’ has incomplete type enum compressed_debug_section_type compress_debug; ^ ../../src/bfd/archive.c: In function ‘open_nested_file’: ../../src/bfd/archive.c:393:12: error: ‘bfd {aka struct bfd}’ has no member named ‘lto_output’ n_bfd->lto_output = archive->lto_output; ^ which appears to be unrelated snafu from the binutils+gdb side. Thanks Dave
On Mon, 2015-11-02 at 11:21 -0500, David Malcolm wrote: > On Sun, 2015-11-01 at 17:06 -0500, Patrick Palka wrote: > > On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener > > <richard.guenther@gmail.com> wrote: > > > On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote: > > >> On 10/29/2015 10:49 AM, David Malcolm wrote: > > >>> > > >>> Our documentation describes -Wall as enabling "all the warnings about > > >>> constructions that some users consider questionable, and that are easy to > > >>> avoid > > >>> (or modify to prevent the warning), even in conjunction with macros." > > >>> > > >>> I believe that -Wmisleading-indentation meets these criteria, and is > > >>> likely to be of benefit to users who may not read release notes; it > > >>> warns for indentation that's misleading, but not for indentation > > >>> that's merely bad: the former are places where a user will likely > > >>> want to fix the code. > > >>> > > >>> The fix is usually easy and obvious: fix the misleadingly-indented > > >>> code. If that isn't an option for some reason, pragmas can be used to > > >>> turn off the warning for a particular fragment of code: > > >>> > > >>> #pragma GCC diagnostic push > > >>> #pragma GCC diagnostic ignored "-Wmisleading-indentation" > > >>> if (flag) > > >>> x = 3; > > >>> y = 2; > > >>> #pragma GCC diagnostic pop > > >>> > > >>> -Wmisleading-indentation has been tested with a variety of indentation > > >>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c) > > >>> and on a variety of real-world projects. For example, in: > > >>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html > > >>> Patrick reports: > > >>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources > > >>> with -Wmisleading-indentation." > > >>> > > >>> With the tweak earlier in this kit I believe we now have a good > > >>> enough signal:noise ratio for this warning to be widely used; hence this > > >>> patch adds the warning to -Wall. > > >>> > > >>> Bootstrapped®rtested with x86_64-pc-linux-gnu. > > >>> > > >>> OK for trunk? > > >>> > > >>> gcc/c-family/ChangeLog: > > >>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++. > > >>> > > >>> gcc/ChangeLog: > > >>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the > > >>> list. > > >>> (-Wmisleading-indentation): Update documentation to reflect > > >>> being enabled by -Wall in C/C++. > > >> > > >> I'm sure we'll get some grief for this :-) > > >> > > >> Approved once we're clean in GCC. I'm going to explicitly say that we'll > > >> have to watch for fallout, particularly as we start getting feedback from > > >> Debian & Fedora mass-rebuilds as we approach release time. If the fallout > > >> is too bad, we'll have to reconsider. > > >> > > >> I'll pre-approve patches which fix anything caught by this option in GCC as > > >> long as the fix just adjusts whitespace :-) > > > > > > Please at least check also binutils and gdb and other packages that use -Werror > > > (well, just rebuild Fedora world). > > > > FYI binutils, gdb and glibc, from git, all fail to build due to > > -Wmisleading-indentation warnings/errors. (None of the warnings are > > bogus (IMO), though I don't think any of the warnings expose a real > > bug either.) > > Bother. Do you happen to have the logs handy? (or could you upload the > somewhere?) > > I tried building binutils+gdb 854eb72b00ba46d65ce36dc3432f01e223ce44cb > with gcc6+this kit (on x86_64) but I get: > In file included from ../../src/bfd/archive.c:143:0: > ../../src/bfd/../include/bfdlink.h:452:38: error: field ‘compress_debug’ > has incomplete type > enum compressed_debug_section_type compress_debug; > ^ > ../../src/bfd/archive.c: In function ‘open_nested_file’: > ../../src/bfd/archive.c:393:12: error: ‘bfd {aka struct bfd}’ has no > member named ‘lto_output’ > n_bfd->lto_output = archive->lto_output; > ^ > which appears to be unrelated snafu from the binutils+gdb side. The only one I saw in glibc was this: ../stdlib/strtol_l.c: In function ‘____strtoul_l_internal’: ../stdlib/strtol_l.c:356:9: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation] cnt < thousands_len; }) ^ ../stdlib/strtol_l.c:353:9: note: ...this ‘for’ clause, but it is not && ({ for (cnt = 0; cnt < thousands_len; ++cnt) ^ where the code is question looks like this: 348 for (c = *end; c != L_('\0'); c = *++end) 349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) c > L_('9')) 350 # ifdef USE_WIDE_CHAR 351 && (wchar_t) c != thousands 352 # else 353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt) 354 if (thousands[cnt] != end[cnt]) 355 break; 356 cnt < thousands_len; }) 357 # endif 358 && (!ISALPHA (c) 359 || (int) (TOUPPER (c) - L_('A') + 10) >= base)) 360 break; It looks like lines 354 and 355 are poorly indented, leading to the warning from -Wmisleading-indentation at line 356. It could be argued that the warning is reasonable here, though I don't like the wording of our warning here: line 356 isn't indented as if guarded by line 353, it's more that lines 354 and 355 *aren't* indented. I've filed PR 68187 to track this.
On Thu, 2015-10-29 at 11:38 -0600, Jeff Law wrote: > On 10/29/2015 10:49 AM, David Malcolm wrote: > > Our documentation describes -Wall as enabling "all the warnings about > > constructions that some users consider questionable, and that are easy to avoid > > (or modify to prevent the warning), even in conjunction with macros." > > > > I believe that -Wmisleading-indentation meets these criteria, and is > > likely to be of benefit to users who may not read release notes; it > > warns for indentation that's misleading, but not for indentation > > that's merely bad: the former are places where a user will likely > > want to fix the code. > > > > The fix is usually easy and obvious: fix the misleadingly-indented > > code. If that isn't an option for some reason, pragmas can be used to > > turn off the warning for a particular fragment of code: > > > > #pragma GCC diagnostic push > > #pragma GCC diagnostic ignored "-Wmisleading-indentation" > > if (flag) > > x = 3; > > y = 2; > > #pragma GCC diagnostic pop > > > > -Wmisleading-indentation has been tested with a variety of indentation > > styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c) > > and on a variety of real-world projects. For example, in: > > https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html > > Patrick reports: > > "Tested by building the linux, git, vim, sqlite and gdb-binutils sources > > with -Wmisleading-indentation." > > > > With the tweak earlier in this kit I believe we now have a good > > enough signal:noise ratio for this warning to be widely used; hence this > > patch adds the warning to -Wall. > > > > Bootstrapped®rtested with x86_64-pc-linux-gnu. > > > > OK for trunk? > > > > gcc/c-family/ChangeLog: > > * c.opt (Wmisleading-indentation): Add to -Wall for C and C++. > > > > gcc/ChangeLog: > > * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the > > list. > > (-Wmisleading-indentation): Update documentation to reflect > > being enabled by -Wall in C/C++. > I'm sure we'll get some grief for this :-) > > Approved once we're clean in GCC. I'm going to explicitly say that > we'll have to watch for fallout, particularly as we start getting > feedback from Debian & Fedora mass-rebuilds as we approach release time. > If the fallout is too bad, we'll have to reconsider. I believe we're now clean; I've committed this to trunk (*without* the blank-lines heuristic/levels idea) as r231539, having bootstrapped®rtested on x86_64-pc-linux-gnu (I've also successfully performed an all-configs build with it, although that was a while back now). > I'll pre-approve patches which fix anything caught by this option in GCC > as long as the fix just adjusts whitespace :-) > > jeff > >
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index e573254..d603511 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -562,7 +562,7 @@ C ObjC C++ ObjC++ Var(warn_memset_transposed_args) Warning LangEnabledBy(C ObjC Warn about suspicious calls to memset where the third argument is constant literal zero and the second is not. Wmisleading-indentation -C C++ Common Var(warn_misleading_indentation) Warning +C C++ Common Var(warn_misleading_indentation) Warning LangEnabledBy(C C++,Wall) Warn when the indentation of the code does not reflect the block structure. Wmissing-braces diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 5665315..abbbe5f 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -3531,6 +3531,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wformat @gol -Wmain @r{(only for C/ObjC and unless} @option{-ffreestanding}@r{)} @gol -Wmaybe-uninitialized @gol +-Wmisleading-indentation @r{(only for C/C++)} @gol -Wmissing-braces @r{(only for C/ObjC)} @gol -Wnonnull @gol -Wopenmp-simd @gol @@ -3876,8 +3877,6 @@ Specifically, a warning is issued for @code{if}, @code{else}, @code{while}, and @code{for} clauses with a guarded statement that does not use braces, followed by an unguarded statement with the same indentation. -This warning is disabled by default. - In the following example, the call to ``bar'' is misleadingly indented as if it were guarded by the ``if'' conditional. @@ -3907,6 +3906,8 @@ The warning is not issued after a @code{#line} directive, since this typically indicates autogenerated code, and no assumptions can be made about the layout of the file that the directive references. +This warning is enabled by @option{-Wall} in C and C++. + @item -Wmissing-braces @opindex Wmissing-braces @opindex Wno-missing-braces
Our documentation describes -Wall as enabling "all the warnings about constructions that some users consider questionable, and that are easy to avoid (or modify to prevent the warning), even in conjunction with macros." I believe that -Wmisleading-indentation meets these criteria, and is likely to be of benefit to users who may not read release notes; it warns for indentation that's misleading, but not for indentation that's merely bad: the former are places where a user will likely want to fix the code. The fix is usually easy and obvious: fix the misleadingly-indented code. If that isn't an option for some reason, pragmas can be used to turn off the warning for a particular fragment of code: #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmisleading-indentation" if (flag) x = 3; y = 2; #pragma GCC diagnostic pop -Wmisleading-indentation has been tested with a variety of indentation styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c) and on a variety of real-world projects. For example, in: https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html Patrick reports: "Tested by building the linux, git, vim, sqlite and gdb-binutils sources with -Wmisleading-indentation." With the tweak earlier in this kit I believe we now have a good enough signal:noise ratio for this warning to be widely used; hence this patch adds the warning to -Wall. Bootstrapped®rtested with x86_64-pc-linux-gnu. OK for trunk? gcc/c-family/ChangeLog: * c.opt (Wmisleading-indentation): Add to -Wall for C and C++. gcc/ChangeLog: * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the list. (-Wmisleading-indentation): Update documentation to reflect being enabled by -Wall in C/C++. --- gcc/c-family/c.opt | 2 +- gcc/doc/invoke.texi | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-)