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

[David Blaikie]

On Tue, Jan 24, 2012 at 8:22 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
> 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.

Little bit of a stab in the dark - but if these are always lexically
scoped, could you use something that worked in a similar way to Sema's
LocalInstantiationScope - the objects go on the stack (not in a list)
& Sema just keeps a pointer to the current one (& the stack keeps the
previous pointer). This may or may not be applicable to this scenario
- but it does seem to have some similarities.

- David

>
> -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
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>