[PATCH] D47303: [analyzer] NFC: Merge object construction related state traits into one.

George Karpenkov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 23 18:17:41 PDT 2018

george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.

Definitely looks much cleaner, some nits inline. Can we protect against API misuse?

Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:769
+  /// that tracks objects under construction.
+  static ProgramStateRef finishObjectConstruction(ProgramStateRef State,
+                                                  const Stmt *S,
Should we somehow assert that those functions are called in the correct order? What will happen if e.g. `finishObjectConstruction` is called twice with the same statement? 

Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:101
-using InitializedTemporariesMap =
-    llvm::ImmutableMap<std::pair<const CXXBindTemporaryExpr *,
-                                 const StackFrameContext *>,
-                       const CXXTempObjectRegion *>;
-// Keeps track of whether CXXBindTemporaryExpr nodes have been evaluated.
-// The StackFrameContext assures that nested calls due to inlined recursive
-// functions do not interfere.
-                                 InitializedTemporariesMap)
-using TemporaryMaterializationMap =
-    llvm::ImmutableMap<std::pair<const MaterializeTemporaryExpr *,
-                                 const StackFrameContext *>,
-                       const CXXTempObjectRegion *>;
-// Keeps track of temporaries that will need to be materialized later.
-// The StackFrameContext assures that nested calls due to inlined recursive
-// functions do not interfere.
-                                 TemporaryMaterializationMap)
-using CXXNewAllocatorValuesMap =
-    llvm::ImmutableMap<std::pair<const CXXNewExpr *, const LocationContext *>,
-                       SVal>;
-// Keeps track of return values of various operator new() calls between
-// evaluation of the inlined operator new(), through the constructor call,
-// to the actual evaluation of the CXXNewExpr.
-// TODO: Refactor the key for this trait into a LocationContext sub-class,
-// which would be put on the stack of location contexts before operator new()
-// is evaluated, and removed from the stack when the whole CXXNewExpr
-// is fully evaluated.
-// Probably do something similar to the previous trait as well.
-                                 CXXNewAllocatorValuesMap)
+// Keeps track of whether objects corresponding to the statement have been
+// fully constructed.
The comment seems incorrect, the map is not only answering the "whether" question, it also maps those pairs into --- (their regions where these objects are constructed into?)

Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:377
+  if (!State->contains<ObjectsUnderConstruction>(Key)) {
+    return State->set<ObjectsUnderConstruction>(Key, V);
nitpick: most C++ containers eliminate the need for two lookups by allowing the `get` method to return a reference. I'm not sure whether this can be done here though.

  rC Clang


More information about the cfe-commits mailing list