On Thu, Mar 3, 2016 at 10:56 AM, David Malcolm <dmalc...@redhat.com> wrote:
> On Thu, 2016-03-03 at 10:24 -0500, Patrick Palka wrote:
>> 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);
>
> No, because the rejection based on indentation is done relative to
> guard_line_first_nws, rather than guard_vis_column (I tried doing it
> via the latter in one version of the patch, and that broke some of the
> existing cases, so I didn't make that change).
I see. That clears things up for me, thanks.
>
> See the attached test file (which doesn't have dg-directives yet); the
> example you give is test1_d, with an open-brace added to the "if (p)".
>
> Trunk emits warnings for:
> * test1_c
> * test1_d
> * test1_e
> * test1_f (two warnings, one for the "if", one for the "else")
> * test1_g
> * test2_c
> * test2_d
> * test2_e
>
> With the patches, it emits warnings for:
> * test1_c
> * test1_d
> * test1_e
> * test1_f (just one warnings, for the "if")
> * test1_g
> * test2_c
> * test2_d
> * test2_e
>
> so the only change is the removal of the erroneous double warning for
> the "else" in test1_f.
Nice!
