On Jan 23, 2012, at 6:40 AM, Dmitri Gribenko wrote:
> On Fri, Jan 20, 2012 at 7:18 AM, Dmitri Gribenko <[email protected]> wrote:
>> On Thu, Jan 19, 2012 at 1:11 AM, Dmitri Gribenko <[email protected]> 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 <[email protected]>*/
> <tree-transform-act-on-block-error.patch><generalize-warn-empty-body-v9.patch>_______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
