On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm <dmalc...@redhat.com> 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.
Wouldn't this change have the undesirable effect of no longer warning about:
if (p)
foo (1);
} else if (q)
foo (2);
foo (3);
