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

[Chandler Carruth]

On Sun, Feb 5, 2012 at 7:39 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> Hello,
>
> Attached is a patch that implements enhancement proposed in PR11329.
>
> The discussion was a bit lengthy, so I'll repeat all design decisions
> to make it easy to follow for reviewers.
>
> Thanks to the reviewers' comments, I've implemented the following
> heuristic that reduces false positives: for and while with null
> statement as a body should be followed by a CompoundStmt in order to
> emit a warning:
>
>   for (int i = 0; i < n; i++);
>   {
>       a(i);
>   }
>
> But applying only the heuristic explained above limits the warning to
> for(...);{...} cases.  The second heuristic is to emit a warning also
> if for/while have less indentation than the next statement, e. g.:
>
>   for (int i = 0; i < n; i++);
>       a(i);
>
> As suggested by Argyrios, we optimize for the common case where there
> are no statements with null bodies.  We implement:
> * a stack of "CompoundScopeInfo"s, helper functions to push/pop them,
> ActOn{Start,Finish}OfCompoundStatement actions;
> * a RAII object CompoundScopeRAII to call ActOn{Start,Finish} actions;
> * a flag in CompoundScopeInfo that records if there were any
> statements null bodies.
>
> I also check if the warning is actually enabled before doing costly checks.
>
> Since this is a syntactic check we don't need to repeatedly emit the
> diagnostic for all template instantiations.  I check
> Sema::CurrentInstantiationScope == NULL (is this the correct
> approach?).
>
> Tests from test/SemaCXX/if-empty-body.cpp are merged with new tests in
> test/SemaCXX/warn-empty-body.cpp.
>
> The patch is the same as v13, but rebased to current trunk.


Most of this is good, but I think we need to white list more cases that are
very unlikely to be bugs, etc.

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.

Both seem very direct and unambiguous. I don't see any really obvious ways
to typo into an empty set of braces -- do we have any bugs for that pattern?

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

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.


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


I'm should get to the code implementing this now eventually, unless someone
else gets there first...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120208/049dc6c2/attachment.html>