[cfe-commits] [PATCH][PR11329][v14] Generalize -Wempty-body: warn when statement body is empty

[Dmitri Gribenko]

Hello Chandler,

Thank you for the review!

On Thu, Feb 9, 2012 at 11:50 AM, Chandler Carruth <chandlerc at google.com> wrote:
> I think there should be two ways to silence this warning:
>
> 1) place the semicolon for the empty statement on its own line, or
> 2) use empty curly braces.

(1) always works, (2) doesn't work only with range-based for.

> Secondly, we shouldn't warn on all range-based for loops: they can have
> side-effects the same as for-loops. I think for consistency they should use
> the same rules for silencing the warning.

I agree that range-based for loops technically can be used only for
side-effects, but that is only possible when iterator's operator++ or
operator!= have side-effects.

Plain for loops with empty bodies are typically used to advance some
iterator to a certain position.  But range-based for loops hide the
iterator from the programmer, so that is not our usecase.

Thus I think that range-based for loops with empty compound statement
should still warn, because in most cases they are effectively a no-op.
 (At least for arrays and STL containers, but I can't think of an
iterator with side-effects useable with a range-based for...)

> I also don't see a point in warning on switch or if statements when the user
> writes one of the very unlikely to be a bug patterns. It might be as simple
> as them commenting out the actual code, and expecting it to still work. It
> seems very unlikely to be an accidental typo.

Well, with this patch if(...) {} still doesn't warn because condition
can contain side-effects (only if(...); warns -- but that was
implemented before).  Nevertheless, I think that even if with braces
should warn and here's why.

If the user consciously wrote that code just for the side-effects in
the condition, then `if' statement is the wrong tool.  More probably,
either that empty body was a typo or one just forgot to populate the
{} with actual body.  If the programmer commented out the body, then
this warning would remind them to uncomment or fix that statement.

If you have some specific examples that are not handled in a sane way
-- I'm all ears.

> Having the same rules for
> silencing a warning on an empty body for all control flow constructs makes
> it much easier to predict how this warning works, which helps users
> understand it.

I agree, that's why there's only a singe exception from the rules:
range-based for with empty braces.

> Finally, a few test cases I'd like to see added:
>
> switch (x) default:; // should not warn -- used in some metaprogramming
> constructs
>
> do {} while (...); // should not warn -- sometimes expanded from macros.

Done, no code changes.

> Extra credit if you can handle this, (hey, maybe it already works! it'd be a
> great test case if so...) but if its hard I would do it in a follow-up
> patch. I imagine this QoI issue to be somewhat complicated to handle:
>
> do;  // should error here due to semi-colon, and synthesize a do-while loop
>   x = 42;
> while (...);  // should not warn here due to "do" above

That would not warn unless next line after while(...); has more
indentation than while itself.  Added the following testcases:

    do;          // expected-note{{to match this 'do'}}
      b();       // expected-error{{expected 'while' in do/while loop}}
    while (b()); // no-warning
    c();

    do;          // expected-note{{to match this 'do'}}
      b();       // expected-error{{expected 'while' in do/while loop}}
    while (b()); // expected-warning{{while loop has empty body}}
expected-note{{put the semicolon on a separate line to silence this
warning}}
      c();

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/