<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi, Jordan<br>
<br>
Thanks for the review, added a post-statement callback to
CXXNewExpr and a pre-statement callback to CXXDeleteExpr as
r170234<br>
<br>
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?<br>
<br>
void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,<br>
ExplodedNodeSet &DstTop) {<br>
...<br>
StmtNodeBuilder Bldr(Pred, DstTop, *currBldrCtx);<br>
<br>
// pre checks<br>
Stmt::StmtClass sc = S->getStmtClass();<br>
if (sc != Stmt::CXXNewExprClass && ...) {<br>
ExplodedNodeSet Tmp;<br>
getCheckerManager().runCheckersForPreStmt(Tmp, Pred, C,
*this);<br>
Bldr.takeNodes(Pred);<br>
Bldr.addNodes(Tmp);<br>
}<br>
...<br>
<br>
// visits<br>
switch (S->getStmtClass()) {<br>
...<br>
case Stmt::CompoundAssignOperatorClass:<br>
VisitBinaryOperator(BO, Bldr);<br>
break;<br>
...<br>
}<br>
...<br>
<br>
// post checks<br>
if (sc != Stmt::CXXDeleteExprClass && ...) {<br>
ExplodedNodeSet Tmp;<br>
getCheckerManager().runCheckersForPreStmt(Tmp, DstTop, S,
*this);<br>
Bldr.takeNodes(DstTop);<br>
Bldr.addNodes(Tmp);<br>
}<br>
}<br>
<br>
void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
StmtNodeBuilder* Bldr);<br>
for (NodeBuilder::iterator i = Bldr.begin(), e = Bldr.end(); i
!= e ; ++i) {<br>
do something;<br>
}<br>
}<br>
</div>
<blockquote
cite="mid:44D9094E-D638-4FD0-BBD0-C5972730B9F4@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div>Hi, Anton. Thanks for looking at this.</div>
<div><br>
</div>
<div>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 <a moz-do-not-send="true"
href="http://llvm.org/bugs/show_bug.cgi?id=12014">PR12014</a>.
There's no real reason for it not to have a post-statement
callback, though.</div>
<div><br>
</div>
<div>What the FIXME was suggesting is not to have individual
pre/post checks in the cases of the Visit method, but to move <i>all</i> the
checks <i>outside</i> the switch statement. The reason <i>this</i> 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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Jordan</div>
<div><br>
</div>
<br>
<div>
<div>On Dec 12, 2012, at 18:37 , Anton Yartsev <<a
moz-do-not-send="true" href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">Hi all,<br>
<br>
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)<br>
it also makes the analyzer invoke [Pre/Post]Stmt checkers
while processing CXXNewExpr and CXXDeleteExpr (that's what the
patch was originally intended for)<br>
Is it Ok to commit the patch?<br>
<br>
-- <br>
Anton<br>
<br>
<span><movePrePostChecksToVisit.patch></span>_______________________________________________<br>
cfe-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote>
</div>
<br>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>