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

[Dmitri Gribenko]

On Thu, Feb 9, 2012 at 4:17 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
> 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();

And here is the corrected patch.

-- 
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>*/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: generalize-warn-empty-body-v15.patch
Type: text/x-diff
Size: 35758 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120209/a79b0016/attachment.patch>