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

[Argyrios Kyrtzidis]

On Jan 24, 2012, at 8:51 PM, David Blaikie wrote:

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

Good point, though not sure it makes much difference in this case, I don't have a strong opinion either way.
But notice that a difference is that LocalInstantiationScope is only internal to Sema, while the ActOnStartOfCompoundStmt/ActOnFinishOfCompoundStmt are used by TreeTransform as well.
But this brings me to another point..

I don't think it make sense to warn on template instantiations, since these are syntactic warnings. For template instantiations they would only contribute noise, e.g:

template <typename T>
void f(bool b) {
	if (b);   // Warn here
}

void g() {
  f<int>(0);  // Warn here as well ?
}

what do you think ?


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