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

[Argyrios Kyrtzidis]

On Jan 23, 2012, at 6:40 AM, Dmitri Gribenko wrote:

> On Fri, Jan 20, 2012 at 7:18 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>> On Thu, Jan 19, 2012 at 1:11 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>>> Attached is a patch that implements enhancement proposed in PR11329.
>>> 
>>> As suggested by Argyrios, this patch implements:
>>> * a stack of "CompoundScopeInfo"s, helper functions to push/pop them,
>>> ActOn{Start,Finish}OfCompoundStatement callbacks;
>>> * a check if the warning is actually enabled before doing costly checks.
>>> 
>>> Doing this uncovered a bug in TreeTransfom, Sema::ActOnBlockError was
>>> not called in all cases.  This is also fixed.
>>> 
>>> I've ran a chromium build again and found 3 false positives (-1
>>> because one previously affected file now builds with -Wno-empty-body).
>> 
>> I accidentally sent an old version with an error.  Here's a correct version.
> 
> *ping*
> 
> Please review.  Patch rebased to latest trunk and split in two.

Sorry for the delay, committed the TreeTransform patch in r148915.

Looks good! Some nitpicks:

-Why did you remove "if-empty-body.cpp" ? Did you include all the test cases in warn-empty-body.cpp ?

> +/// \brief Contains information about the compound statement currently being
> +/// parsed.
> +class CompoundScopeInfo {
> +public:
> +  CompoundScopeInfo()
> +    : HasEmptyLoopBodies(false) { }
> +
> +  /// \brief Whether this compound stamement contains `for' or `while' loops
> +  /// with empty bodies.
> +  bool HasEmptyLoopBodies;
> +
> +  void setHasEmptyLoopBodies() {
> +    HasEmptyLoopBodies = true;
> +  }
> +};
> +

You have a public field and a setter for it. Did you mean to actually make the field private so you cannot "unset" it ?

> +  SmallVector<CompoundScopeInfo *, 4> CompoundScopes;

Why don't you push CompoundScopes as value objects, to avoid malloc traffic ?
to be more specific:

SmallVector<CompoundScopeInfo, 4> CompoundScopes;
CompoundScopeInfo &getCurCompoundScope() const;


> -  
> +
> +  ActOnStartOfCompoundStmt();
>    StmtResult Body = ActOnCompoundStmt(Loc, Loc, move_arg(Statements),
>                                              /*isStmtExpr=*/false);
> +  ActOnFinishOfCompoundStmt();

How about using a RAII object to make this less error prone, say something like a CompoundScopeRAII class, that will do the calls in the constructor/destructor.
This would be particularly useful inside TreeTransform<Derived>::TransformCompoundStmt.

-Argyrios


> 
> 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>*/
> <tree-transform-act-on-block-error.patch><generalize-warn-empty-body-v9.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120124/e74fd466/attachment.html>