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

[George Karpenkov via Phabricator via cfe-commits]

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.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporaries,
-                                 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.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(TemporaryMaterializations,
-                                 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.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(CXXNewAllocatorValues,
-                                 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.


Repository:
  rC Clang

https://reviews.llvm.org/D47303