I like that this actually isn't that complicated! Can you add some tests for
new[], with placement args?
================
Comment at: include/clang/Analysis/CFG.h:148
@@ +147,3 @@
+public:
+ CFGNewAllocator(const CXXNewExpr *S)
+ : CFGElement(NewAllocator, S) {}
----------------
Nitpick: this should be marked `explicit`.
================
Comment at: include/clang/Analysis/CFG.h:57
@@ -55,2 +56,3 @@
Initializer,
+ NewAllocator,
// dtor kind
----------------
We're going to run out of Kinds some day. Can you put an assertion in the
CFGElement constructor that the stored Kind is the same as the argument Kind?
================
Comment at: lib/Analysis/CFG.cpp:3140-3142
@@ +3139,5 @@
+ Block = VisitStmt(NE->getInitializer(), asc);
+ if (NE->isArray())
+ Block = VisitStmt(NE->getArraySize(), asc);
+ appendNewAllocator(Block, NE);
+ if (NE->getNumPlacementArgs() > 0) {
----------------
I think this is backwards, since the array size is used to determine the
allocation size. We aren't likely to start using the array size right away,
though.
================
Comment at: lib/Analysis/CFG.cpp:3143-3147
@@ +3142,7 @@
+ appendNewAllocator(Block, NE);
+ if (NE->getNumPlacementArgs() > 0) {
+ for (int i=0;i<NE->getNumPlacementArgs();++i) {
+ Block = VisitStmt(NE->getPlacementArg(i), asc);
+ }
+ }
+ return Block;
----------------
I think it's better to loop over placement_arg_begin/placement_arg_end. This
code relies on the placement args being stored in a way that's efficient for
random access...which is true today, but may not always be true.
Also, please include spaces after the semicolons in the loops.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:556
@@ +555,3 @@
+ ExplodedNodeSet Dst;
+ VisitCXXNewAllocatorCall(NE,Pred,Dst);
+ Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
----------------
Please insert spaces after the comma here.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:334
@@ +333,3 @@
+ ExplodedNodeSet &Dst) {
+ //TODO: Handle VisitCXXNewAllocatorExpr
+}
----------------
I would think the easiest way to skip the allocator would be `Dst.insert(Pred)`
and leave it at that.
================
Comment at: lib/Analysis/CFG.cpp:3817
@@ -3792,1 +3816,3 @@
+ } else if (Optional<CFGNewAllocator> NE = E.getAs<CFGNewAllocator>()) {
+ OS << "CFGNewAllocator\n";
} else if (Optional<CFGDeleteDtor> DE = E.getAs<CFGDeleteDtor>()) {
----------------
Nitpick: we can do better. Maybe put the new-expression in parens, or at least
the type being allocated?
http://llvm-reviews.chandlerc.com/D2423
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
