Re: [PATCH 3/3] Improve -Wmissing-indentation heuristics

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

On Thu, Jun 18, 2015 at 12:31 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2015-06-09 at 13:31 -0400, Patrick Palka wrote:
>> This patch improves the heuristics of the warning in a number of ways.
>> The improvements are hopefully adequately documented in the code
>> comments.
>>
>> The additions to the test case also highlight the improvements.
>>
>> I tested an earlier version of this patch on more than a dozen C code
>> bases.  I only found one class of bogus warnings yet emitted, in the
>> libpng and bdwgc projects.  These projects have a coding style which
>> indents code inside #ifdefs as if this code was guarded by an if(), e.g.
>>
>>   if (foo != 0)
>>     x = 10;
>>   else       // GUARD
>>     y = 100; // BODY
>>
>>   #ifdef BAR
>>     blah ();  // NEXT
>>   #endif
>
> We have detect_preprocessor_logic which suppresses warnings when there's
> preprocessor logic between BODY_EXPLOC and NEXT_STMT_EXPLOC, for cases
> like this:
>
>         if (flagA)
>           foo ();
>           ^ BODY_EXPLOC
>       #if SOME_CONDITION_THAT_DOES_NOT_HOLD
>         if (flagB)
>       #endif
>           bar ();
>           ^ NEXT_STMT_EXPLOC
>
> This currently requires that it be multiline preprocessor logic: there
> must be 3 or more lines between BODY_EXPLOC and NEXT_STMT_EXPLOC for
> this rejection heuristic to fire.

Oh I now see why it requires 3 or more lines: one line each for the
#if, #endif and for the

>
> Perhaps we could tweak or reuse that heuristic, perhaps if there's an
> entirely blank (or all whitespace) line separating them (given that this
> warning is all about the "look" of the code).

That makes sense.  What about just checking in
detect_preprocessor_logic() if there is > 1 line (instead of >= 3
lines) between the body and the next statement?  When that's the case,
then whatever is in between the start of the body must either be more
of the body (if it's a multi-line compound statement) or whitespace.
In either case we should not warn if the next statement is aligned
with the body.  Yet we will still rightfully warn on the following
code:

    if (foo)  // GUARD
      bar ();  // BODY
    #ifdef BAZ
      baz ();  // NEXT
    #endif

because there is just one line between the body and the next
statement. The user can add a line between the body and the next
statement to suppress the warning if it's bogus.

>
>> These bogus warnings are pre-existing, however (i.e. not caused by this
>> patch).
>
> (nods)   Fixing the false positives from libpng/bdwgc sounds like a
> separate issue and thus a separate patch then.
>
> [...snip...]
>
>