[PATCH] Proposal on how to fix temporary dtors.

Jordan Rose jordan_rose at apple.com
Tue May 27 11:09:18 PDT 2014


Maybe this is a bit paranoid, but I'm confused as to how we can only save one `LastBindTemporary` value instead of a stack. What's the purpose of that particular variable again? It seems to be used for more than just "if there are no temporaries, we don't need a cleanup block".

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:98-99
@@ -97,2 +97,4 @@
                     ExplodedNode *Pred);
+  void HandleBindTemporary(const CXXBindTemporaryExpr *BTE, const CFGBlock *B,
+                           ExplodedNode *Pred);
 
----------------
Now that I think about it, this name is wrong. It's really "HandleCleanupTemporaryBranch" or something. The CXXBindTemporaryExpr just tells you which temporary it is. (And similar for similarly-named functions.)

================
Comment at: lib/Analysis/CFG.cpp:429-438
@@ -422,3 +428,12 @@
                                             bool BindToTemporary);
+  // Adds cleanup logic for temporaries in case of a conditional path.
+  //
+  // 1. Does nothing if 'E' does not construct temporary objects.
+  // 2. Equivalent to VisitForTemporaryDtors if 'E' contains temporary
+  //    destructors, but they are already handled in a branch due to a child
+  //    statement of 'E'.
+  // 3. Otherwise, creates a new decision node that conditionally executes the
+  //    temporary destructor and routes back to the current block on entry.
+  CFGBlock *VisitForTemporaryDtorsBranch(Stmt *E, bool BindToTemporary);
 
   // NYS == Not Yet Supported
----------------
You might as well make this a proper doc comment.

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:766-767
@@ +765,4 @@
+      if (getAnalysisManager().options.includeTemporaryDtorsInCFG()) {
+        VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), Pred, PreVisit,
+                                  Next);
+      } else {
----------------
Calling this VisitCXXBindTemporaryExpr but then only having it actually be called when the flag is set seems like a bad idea if anyone ever needs to add behavior that should happen all the time. Either the check should be moved inside the method or it should be called something else.

================
Comment at: test/Analysis/temporaries.cpp:316
@@ +315,3 @@
+      ++y;
+      if (true) (void)0; else (void)check(NoReturnDtor());
+    }
----------------
This and the `true ? ...` below will get optimized out at CFG construction time. If that's correct, you should comment it; if you want to test path sensitivity, you should take a boolean input variable and then fully constrain it at the top of the function.

http://reviews.llvm.org/D3627






More information about the cfe-commits mailing list