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

[Jordan Rose]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121213/69f323bf/attachment.html>