[PATCH] Append CXXDefaultInitExpr's wrapped expression to the CFG when visiting a constructor initializer

[Enrico Pertoso]

================
Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:282-294
@@ -267,6 +281,15 @@
       } else if (End && isa<CXXDefaultInitExpr>(End)) {
-        PathPieces::iterator Next = llvm::next(I);
-        if (Next != E) {
-          if (PathDiagnosticControlFlowPiece *NextCF =
-                dyn_cast<PathDiagnosticControlFlowPiece>(*Next)) {
-            NextCF->setStartLocation(CF->getStartLocation());
+        const Expr *InitExpr = cast<CXXDefaultInitExpr>(End)->getExpr();
+        if (Start && containsStmt(InitExpr, Start)) {
+          if (I != Pieces.begin()) {
+            PathPieces::iterator Prev = llvm::prior(I);
+            if (PathDiagnosticControlFlowPiece *PrevCF =
+                    dyn_cast<PathDiagnosticControlFlowPiece>(*Prev)) {
+              CF->setStartLocation(PrevCF->getStartLocation());
+              Start = CF->getStartLocation().asStmt();
+              Pieces.erase(Prev);
+              continue;
+            }
+          }
+        } else {
+          PathPieces::iterator Next = llvm::next(I);
----------------
Jordan Rose wrote:
> Enrico Pertoso wrote:
> > Jordan Rose wrote:
> > > This definitely needs testing. The whole point of this section is to make Xcode arrows prettier, so without a test case that actually includes a plist it's kind of useless.
> > > 
> > > If you have Xcode, you can test the new arrows by overriding the analyzer binary with the not-so-secret setting CLANG_ANALYZER_EXEC. If not, don't worry about the arrows for now; I'll get to them soon.
> > `test/Analysis/edges-new.mm` is actually a plist based test and without changes to this section it was failing (a `CXXDefaultInitExpr` may now include the field initializer, so it may spawn more than a `PathDiagnosticControlFlowPiece`).
> > 
> > I modified it a little bit (inserting a conditional operator in `edges-new.mm` at line `580`) to make sure that there's no arrow pointing to (or starting from) the actual init expression.
> What I mean is that we need some positive tests here, such as tests where an error happens within the default expr. If you change a plist test without the plist output changing, you're very likely not testing anything new.
I added a null-pointer dereference at line 594 in edges-new.mm. This dereference happens in the default initializer expression when the value of another variable is set in the constructor.

The static analyzer now correctly detects the null-pointer dereference on that line. I'm still not 100% happy because, in the presence of multiple constructors, it's not immediately clear which one of them leads to the warning, but at least it won't go unnoticed.

There were indeed edges to the in-class initializer: I'm not sure what the correct behaviour should be in this regard. I tried to be conservative and the current patch prevents them from appearing.

================
Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:251-261
@@ +250,13 @@
+/// the second statement.
+static bool containsStmt(const Stmt *Ex, const Stmt *S) {
+  if (Ex == S)
+    return true;
+
+  for (Stmt::const_child_iterator I = Ex->child_begin(), E = Ex->child_end();
+       I != E; ++I) {
+    if (containsStmt(*I, S))
+      return true;
+  }
+  return false;
+}
+
----------------
Jordan Rose wrote:
> Enrico Pertoso wrote:
> > Jordan Rose wrote:
> > > Usually we do this using ParentMap so that it's a linear search. We'd have to start adding the children of the CXXDefaultInitExpr to the ParentMap, though.
> > We don't actually need to know a statement's parent, I used a `llvm::DenseSet` instead. Unlike the ParentMap, it can be cleared.
> I'm not sure why this is an improvement. You don't even get to short-circuit here, and you rebuild the set every time a path piece goes through this default init expr. What if the same constructor is called several times in the course of analyzing a top-level function?
I'm using the `ParentMap` returned by `AnalysisDeclContext::getParentMap()` now. I think it makes more sense. 
Do you see any problem with that?



http://llvm-reviews.chandlerc.com/D2370