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.guent...@gmail.com> wrote:
> > > On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <l...@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.
