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

[David Malcolm <dmalcolm@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]

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).

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.

I can add dg-directives and add the attachment to Wmisleading
-indentation.c as part of the patch (or keep it as a separate new test
file, the former is getting large).

[-- Attachment #2: Wmisleading-indentation-4.c --]
[-- Type: text/x-csrc, Size: 1830 bytes --]

/* PR c/68187.  */
/* { dg-options "-Wmisleading-indentation" } */
/* { dg-do compile } */

extern int foo (int);
extern int bar (int, int);
extern int flagA;
extern int flagB;
extern int flagC;
extern int flagD;

/* Before the "}".  */

void
test1_a (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
 foo (2);
 foo (3);
}

/* Aligned with the "}".  */

void
test1_b (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
  foo (2);
  foo (3);
}

/* Indented between the "}" and the "else".  */

void
test1_c (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
   foo (2);
   foo (3);
}

/* Aligned with the "else".  */

void
test1_d (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
    foo (2);
    foo (3);
}

/* Indented between the "else" and the "if".  */

void
test1_e (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
      foo (2);
      foo (3);
}

/* Aligned with the "if".  */

void
test1_f (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
         foo (2);
         foo (3);
}

/* Indented more than the "if".  */

void
test1_g (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
            foo (2);
            foo (3);
}

/* Again, but without the 2nd "if".  */

/* Before the "}".  */

void
test2_a (void)
{
  if (flagA) {
    foo (1);
  } else
 foo (2);
 foo (3);
}

/* Aligned with the "}".  */

void
test2_b (void)
{
  if (flagA) {
    foo (1);
  } else
  foo (2);
  foo (3);
}

/* Indented between the "}" and the "else".  */

void
test2_c (void)
{
  if (flagA) {
    foo (1);
  } else
   foo (2);
   foo (3);
}

/* Aligned with the "else".  */

void
test2_d (void)
{
  if (flagA) {
    foo (1);
  } else
    foo (2);
    foo (3);
}

/* Indented more than the "else".  */

void
test2_e (void)
{
  if (flagA) {
    foo (1);
  } else
        foo (2);
        foo (3);
}