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).
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).
/* 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);
}
