[PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)
Jeff Law
law@redhat.com
Fri Mar 4 07:15:00 GMT 2016
On 03/03/2016 08:21 AM, David Malcolm wrote:
> PR c/68187 covers two cases involving poor indentation where
> the indentation is arguably not misleading, but for which
> -Wmisleading-indentation emits a warning.
>
> The two cases appear to be different in nature; one in comment #0
> and the other in comment #1. Richi marked the bug as a whole as
> a P1 regression; it's not clear to me if he meant one or both of
> these cases, so the following two patches fix both.
>
> The rest of this post addresses the case in comment #0 of the PR;
> the followup post addresses the other case, in comment #1 of the PR.
>
> Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64
> led to this diagnostic from -Wmisleading-indentation:
>
> ../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)
> ^
>
> 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;
>
> Lines 354 and 355 are poorly indented, leading to the warning from
> -Wmisleading-indentation at line 356.
>
> The wording of the warning is clearly wrong: line 356 isn't indented as if
> guarded by line 353, it's more that lines 354 and 355 *aren't* indented.
>
> What's happening is that should_warn_for_misleading_indentation has a
> heuristic for handling "} else", such as:
>
> if (p)
> foo (1);
> } else // GUARD
> foo (2); // BODY
> foo (3); // NEXT
>
> and this heuristic uses the first non-whitespace character in the line
> containing GUARD as the column of interest: the "}" character.
>
> In this case we have:
>
> 353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt) // GUARD
> 354 if (thousands[cnt] != end[cnt]) // BODY
> 355 break;
> 356 cnt < thousands_len; }) // NEXT
>
> and so it uses the column of the "&&", and treats it as if it were
> indented thus:
>
> 353 for (cnt = 0; cnt < thousands_len; ++cnt) // GUARD
> 354 if (thousands[cnt] != end[cnt]) // BODY
> 355 break;
> 356 cnt < thousands_len; }) // NEXT
>
> and thus issues a warning.
>
> As far as I can tell the heuristic in question only makes sense for
> "else" clauses, so the following patch updates it to only use the
> special column when handling "else" clauses, eliminating the
> overzealous warning.
>
> Doing so led to a nonsensical warning for
> libstdc++-v3/src/c++11/random.cc:random_device::_M_init:
>
> random.cc: In member function âvoid std::random_device::_M_init(const string&)â:
> random.cc:102:10: warning: this âifâ clause does not guard... [-Wmisleading-indentation]
> else if (token != "/dev/urandom" && token != "/dev/random")
> ^~
> random.cc:107:5: note: ...this statement, but the latter is indented as if it does
> _M_file = static_cast<void*>(std::fopen(fname, "rb"));
> ^~~~~~~
>
> so the patch addresses this by tweaking the heuristic that rejects
> aligned BODY and NEXT so that it doesn't require them to be aligned with
> the first non-whitespace of the GUARD, simply that they not be indented
> relative to it.
>
> Successfully bootstrapped®rtested on x86_64-pc-linux-gnu in
> combination with the following patch; standalone bootstrap®rtest
> is in progress.
>
> OK for trunk if the latter is successful?
>
> gcc/c-family/ChangeLog:
> PR c/68187
> * c-indentation.c (should_warn_for_misleading_indentation): When
> suppressing warnings about cases where the guard and body are on
> the same column, only use the first non-whitespace column in place
> of the guard token column when dealing with "else" clauses.
> When rejecting aligned BODY and NEXT, loosen the requirement
> from equality with the first non-whitespace of guard to simply
> that they not be indented relative to it.
>
> gcc/testsuite/ChangeLog:
> PR c/68187
> * c-c++-common/Wmisleading-indentation.c (fn_40_a): New test
> function.
> (fn_40_b): Likewise.
> (fn_41_a): Likewise.
> (fn_41_b): Likewise.
OK.
FWIW, I think all of c-indentation would fall under the diagnostics
maintainership you picked up recently.
jeff
More information about the Gcc-patches
mailing list