[PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)

[Jeff Law]

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&regrtested on x86_64-pc-linux-gnu in
> combination with the following patch; standalone bootstrap&regrtest
> 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