[PATCH] Enhance CFG to model C++ new more precisely

[Jordan Rose]

  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