[PATCH] Implement inlining of temporary destructors.

Jordan Rose jordan_rose at apple.com
Fri Aug 8 18:16:08 PDT 2014


Do we really need to put the "binds parameter" information in the CFG? I was hoping we could look in the parent map to see how the temporary is used (like for 'new' right now).

================
Comment at: include/clang/Analysis/CFG.h:285
@@ -285,1 +284,3 @@
+  CFGTemporaryDtor(CXXBindTemporaryExpr *expr, bool BindsParameter)
+      : CFGImplicitDtor(TemporaryDtor, expr, BindsParameter ? T : nullptr) {}
 
----------------
Why not just use "self"? Or &T? Why "T"? Why does T require a dynamic allocation?

================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:688-693
@@ +687,8 @@
+  const MemRegion *Region = nullptr;
+  // If the class does not have any members, there will not be a region
+  // for it bound in the environment.
+  if (Optional<nonloc::LazyCompoundVal> LCV =
+          Val.getAs<nonloc::LazyCompoundVal>()) {
+    Region = LCV->getRegion();
+  }
+
----------------
This is not how LCVs work. An LCV is a snapshot of a region at a particular "time" (state of the Store); the region may not exist in the current store. The "best" answer in the current scheme is to use createTemporaryRegionIfNeeded, but the right answer may end up being to disregard the rvalue-ness of temporaries (as we talked about some on cfe-dev).

================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:508-509
@@ -507,13 +507,4 @@
 
 bool ScanReachableSymbols::scan(nonloc::LazyCompoundVal val) {
-  bool wasVisited = !visited.insert(val.getCVData()).second;
-  if (wasVisited)
-    return true;
-
-  StoreManager &StoreMgr = state->getStateManager().getStoreManager();
-  // FIXME: We don't really want to use getBaseRegion() here because pointer
-  // arithmetic doesn't apply, but scanReachableSymbols only accepts base
-  // regions right now.
-  const MemRegion *R = val.getRegion()->getBaseRegion();
-  return StoreMgr.scanReachableSymbols(val.getStore(), R, *this);
+  return scan(val.getRegion());
 }
----------------
This is not correct; see above.

http://reviews.llvm.org/D4740






More information about the cfe-commits mailing list