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

[Patrick Palka <patrick@parcs.ath.cx>]

On Thu, Mar 3, 2016 at 10:56 AM, David Malcolm <dmalcolm@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 <dmalcolm@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!