This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall


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&regrtested 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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]