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: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);