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
