[cfe-commits] [PATCH][Review request] move Pre/Post checks to the common Visit() method
Anton Yartsev
anton.yartsev at gmail.com
Fri Dec 14 12:20:44 PST 2012
Hi, Jordan
Thanks for the review, added a post-statement callback to CXXNewExpr and
a pre-statement callback to CXXDeleteExpr as r170234
I intend to finish the refactoring. The scratch below illustrates how
the code will approximately look like after the refactoring. Have I got
the idea?
void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
ExplodedNodeSet &DstTop) {
...
StmtNodeBuilder Bldr(Pred, DstTop, *currBldrCtx);
// pre checks
Stmt::StmtClass sc = S->getStmtClass();
if (sc != Stmt::CXXNewExprClass && ...) {
ExplodedNodeSet Tmp;
getCheckerManager().runCheckersForPreStmt(Tmp, Pred, C, *this);
Bldr.takeNodes(Pred);
Bldr.addNodes(Tmp);
}
...
// visits
switch (S->getStmtClass()) {
...
case Stmt::CompoundAssignOperatorClass:
VisitBinaryOperator(BO, Bldr);
break;
...
}
...
// post checks
if (sc != Stmt::CXXDeleteExprClass && ...) {
ExplodedNodeSet Tmp;
getCheckerManager().runCheckersForPreStmt(Tmp, DstTop, S, *this);
Bldr.takeNodes(DstTop);
Bldr.addNodes(Tmp);
}
}
void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
StmtNodeBuilder* Bldr);
for (NodeBuilder::iterator i = Bldr.begin(), e = Bldr.end(); i != e ;
++i) {
do something;
}
}
> Hi, Anton. Thanks for looking at this.
>
> The reason CXXNewExpr does not currently have pre- and post- statement
> checks is because it's not clear what a pre-statement check means. By
> the time the analyzer reaches the CXXNewExpr, the constructor for the
> object has already been run (because the CXXConstructExpr is a child
> expression). This is already a problem for custom allocators (operator
> new). My plan for this is to add a new CFG node to represent the
> allocator call, but I haven't gotten around to it. You can see the
> discussion (from months ago, sadly) in PR12014
> <http://llvm.org/bugs/show_bug.cgi?id=12014>. There's no real reason
> for it not to have a post-statement callback, though.
>
> What the FIXME was suggesting is not to have individual pre/post
> checks in the cases of the Visit method, but to move /all/ the checks
> /outside/ the switch statement. The reason /this/ has been delayed is
> because we were supposed to switch from passing around
> ExplodedNodeSets to passing around NodeBuilders, but even without that
> we'd probably still want to be passing in ExplodedNodeSets rather than
> a single ExplodedNode predecessor, and push the loops as far inwards
> as possible.
>
> There are a couple of other statements where we deliberately skipped
> the pre- or post- statement callback. I think ReturnStmt is one, since
> a post-statement check isn't exactly meaningful. But we can probably
> special-case those just before calling out to the CheckerManager.
>
> So with all that said, if you want to perpetuate the current state of
> affairs and add a post-statement callback to CXXNewExpr and a
> pre-statement callback to CXXDeleteExpr, that's all right. (I can
> guess you're looking at writing some kind of new/delete checker, which
> we could really use.) But if you're going for the full refactoring,
> let's make sure we get it right.
>
> Jordan
>
>
> On Dec 12, 2012, at 18:37 , Anton Yartsev <anton.yartsev at gmail.com
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> Hi all,
>>
>> the patch moves all the [Pre/Post]checks from paticular
>> VisitSomething() methods to the common Visit() method (the idea for
>> refactoring came from FIXME notes)
>> it also makes the analyzer invoke [Pre/Post]Stmt checkers while
>> processing CXXNewExpr and CXXDeleteExpr (that's what the patch was
>> originally intended for)
>> Is it Ok to commit the patch?
>>
>> --
>> Anton
>>
>> <movePrePostChecksToVisit.patch>_______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121215/457bd25a/attachment.html>
More information about the cfe-commits
mailing list