[cfe-commits] [PATCH][Review request] move Pre/Post checks to the common Visit() method

[Anton Yartsev]

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>