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

[Chandler Carruth]

On Thu, Feb 9, 2012 at 4:17 AM, 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,


The very first testcase in your patch shows that it does not always work:

Index: test/Analysis/PR9741.cpp
===================================================================
--- test/Analysis/PR9741.cpp (revision 150173)
+++ test/Analysis/PR9741.cpp (working copy)
@@ -4,5 +4,5 @@
   int a[] = { 1, 2, 3 };
   unsigned int u = 0;
   for (auto x : a)
-    ;
+    ; // expected-warning{{range-based for loop has empty body}}
 }



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

As Richard has pointed out, there are many ways where one might observe
side-effects. However, I think (and he seemed to agree) that the primary
motivation is consistency. We should have one rule for all these constructs
unless there is an extremely clear reason to have a special case.


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

I think you're missing my point. I'm trying to make this warning target
bugs and mistakes. I don't think it is important or even really advisable
for Clang to warn about code which the user very clearly wrote in a
particular way even if that formulation is strange or confusing. Quite
frankly, it's not the role of the compiler.

The rationale for warning on 'if (...);' iff the following line is indented
differently is because it is a very easy typo or mistake to make, not
because of any moral objection to empty bodies on ifs.

I'm quite concerned about the following not warning:

if (...) { /*my_silly_code();*/ }
if (...) {
  // my_silly_code();
}

Whether or not you would like the warning on these to remind you to
un-comment the code, many do not. If you build with -Werror, this warning
becomes useless for you. Even if not, such commented out code should be
possible to check in and keep around if needed. It's not my style or
convention, but I don't think Clang should get into the business of
dictating these patterns.


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

I'm specifically asking for this warning to not fire. It's not clear that
its going to be correct in the presence of the error before, and it would
be better to teach Clang to recover from the error above by associating it
with the while here. Again, I'm fine for this to be a FIXME for now.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120209/ed1245e1/attachment.html>