On Mon, 2015-11-02 at 13:39 -0500, Patrick Palka wrote:
> On Mon, 2 Nov 2015, 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.
> >
> > Thanks
> > Dave
> >
> >
>
> I don't have build logs but I have diffs that indicates where in the
> code the warnings appear and how to address the warnings (roughly).
Thanks.
> For binutils-gdb:
>
> diff --git a/bfd/coff-i386.c b/bfd/coff-i386.c
> index a9725c4..fca7887 100644
> --- a/bfd/coff-i386.c
> +++ b/bfd/coff-i386.c
> @@ -139,7 +139,7 @@ coff_i386_reloc (bfd *abfd,
> #define DOIT(x) \
> x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) &
> howto->dst_mask))
>
> - if (diff != 0)
> + if (diff != 0)
> {
> reloc_howto_type *howto = reloc_entry->howto;
> unsigned char *addr = (unsigned char *) data + reloc_entry->address;
This one has a fully blank line, so patch [1/4] would have suppressed
it.
> diff --git a/bfd/coff-x86_64.c b/bfd/coff-x86_64.c
> index 4e6420a..23ffecb 100644
> --- a/bfd/coff-x86_64.c
> +++ b/bfd/coff-x86_64.c
> @@ -138,7 +138,7 @@ coff_amd64_reloc (bfd *abfd,
> #define DOIT(x) \
> x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) &
> howto->dst_mask))
>
> - if (diff != 0)
> + if (diff != 0)
> {
> reloc_howto_type *howto = reloc_entry->howto;
> unsigned char *addr = (unsigned char *) data + reloc_entry->address;
Likewise.
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index fff4862..2559a36 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -11359,9 +11359,11 @@ ada_evaluate_subexp (struct type *expect_type,
> struct expression *exp,
> return value_zero (ada_aligned_type (type), lval_memory);
> }
> else
> - arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
> - arg1 = unwrap_value (arg1);
> - return ada_to_fixed_value (arg1);
> + {
> + arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
> + arg1 = unwrap_value (arg1);
> + return ada_to_fixed_value (arg1);
> + }
>
> case OP_TYPE:
> /* The value is not supposed to be used. This is here to make it
Interesting. It's not technically a bug, since the "if true" clause has
an unconditional return, but it looks like a time-bomb to me. I'm happy
that we warn for it.
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 1af477c..1bd3b12 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -1305,26 +1305,26 @@ c_type_print_base (struct type *type, struct ui_file
> *stream,
> if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
> fprintf_filtered (stream, "\n");
>
> - for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> - {
> - struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> -
> - /* Dereference the typedef declaration itself. */
> - gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> - target = TYPE_TARGET_TYPE (target);
> -
> - print_spaces_filtered (level + 4, stream);
> - fprintf_filtered (stream, "typedef ");
> -
> - /* We want to print typedefs with substitutions
> - from the template parameters or globally-known
> - typedefs but not local typedefs. */
> - c_print_type (target,
> - TYPE_TYPEDEF_FIELD_NAME (type, i),
> - stream, show - 1, level + 4,
> - &semi_local_flags);
> - fprintf_filtered (stream, ";\n");
> - }
> + for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> + {
> + struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> +
> + /* Dereference the typedef declaration itself. */
> + gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> + target = TYPE_TARGET_TYPE (target);
> +
> + print_spaces_filtered (level + 4, stream);
> + fprintf_filtered (stream, "typedef ");
> +
> + /* We want to print typedefs with substitutions
> + from the template parameters or globally-known
> + typedefs but not local typedefs. */
> + c_print_type (target,
> + TYPE_TYPEDEF_FIELD_NAME (type, i),
> + stream, show - 1, level + 4,
> + &semi_local_flags);
> + fprintf_filtered (stream, ";\n");
> + }
> }
Likewise, the blank line means patch [1/4] would have suppressed it.
> fprintfi_filtered (level, stream, "}");
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index d38a43d..87988ea 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -413,7 +413,7 @@ child_terminal_ours_1 (int output_only)
> if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
> return;
>
> - {
> + {
> #ifdef SIGTTOU
> /* Ignore this signal since it will happen when we try to set the
> pgrp. */
> @@ -497,7 +497,7 @@ child_terminal_ours_1 (int output_only)
> result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> #endif
> - }
> + }
> }
again, another blank line.
> /* Per-inferior data key. */
> diff --git a/gdb/linux-record.c b/gdb/linux-record.c
> index 091ac8a..18f8fbf 100644
> --- a/gdb/linux-record.c
> +++ b/gdb/linux-record.c
> @@ -112,7 +112,7 @@ record_linux_sockaddr (struct regcache *regcache,
> "memory at addr = 0x%s len = %d.\n",
> phex_nz (len, tdep->size_pointer),
> tdep->size_int);
> - return -1;
> + return -1;
> }
> addrlen = (int) extract_unsigned_integer (a, tdep->size_int, byte_order);
> if (addrlen <= 0 || addrlen > tdep->size_sockaddr)
[...snip...]
These three are all of the form:
if (record_debug)
fprint (...multiple lines...);
return -1;
It seems reasonable to me to complain about the indentation of the
return -1;
> ====
>
> And for glibc:
>
> diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
> index 1382eb5..1963c31 100644
> --- a/stdio-common/vfscanf.c
> +++ b/stdio-common/vfscanf.c
> @@ -1711,7 +1711,7 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format,
> _IO_va_list argptr,
>
> /* The last thousands character will be added back by
> the char_buffer_add below. */
> - --charbuf.current;
> + --charbuf.current;
> #endif
> }
> else
Another one where there's a blank line, which 1/4 would have suppressed.
> diff --git a/stdlib/strtol_l.c b/stdlib/strtol_l.c
> index 8f6163d..0fb9a92 100644
> --- a/stdlib/strtol_l.c
> +++ b/stdlib/strtol_l.c
> @@ -351,8 +351,8 @@ INTERNAL (__strtol_l) (const STRING_TYPE *nptr,
> STRING_TYPE **endptr,
> && (wchar_t) c != thousands
> # else
> && ({ for (cnt = 0; cnt < thousands_len; ++cnt)
> - if (thousands[cnt] != end[cnt])
> - break;
> + if (thousands[cnt] != end[cnt])
> + break;
> cnt < thousands_len; })
> # endif
> && (!ISALPHA (c)
This is the one I filed earlier today as PR 68187.
> diff --git a/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> b/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> index 0c7685c..a0d844c 100644
> --- a/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> +++ b/sysdeps/ieee754/flt-32/k_rem_pio2f.c
> @@ -65,7 +65,8 @@ int __kernel_rem_pio2f(float *x, float *y, int e0, int nx,
> int prec, const int32
>
> /* compute q[0],q[1],...q[jk] */
> for (i=0;i<=jk;i++) {
> - for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j]; q[i] = fw;
> + for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j];
> + q[i] = fw;
> }
>
> jz = jk;
I think it's very reasonable to complain about the above code.
So if I've counted things right the tally is:
* 5 dubious-looking though correct places, where it's reasonable to
issue a warning
* 5 places where there's a blank line between guarded and non-guarded
stmt, where patch 1 of the kit would have suppressed the warning
* one bug (PR 68187)
I think we want the first kind of thing at -Wall, but I'm not so keen on
the second kind at -Wall. Is there precedent for "levels" of a warning?
(so e.g. pedantry level 1 at -Wall, level 2 at -Wextra, and have patch 1
be the difference between levels 1 and 2?)
Sorry for harping on about patch 1; thanks again for posting this
Dave
