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

Jordan Rose jordan_rose at apple.com
Wed Dec 11 11:08:37 PST 2013



================
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);
----------------
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.

================
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;
+}
+
----------------
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?


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



More information about the cfe-commits mailing list