[clang][dataflow] Move logic for `createStorageLocation` from `DataflowEnvironment` to `DataflowAnalysisContext`.

[wyt]

Summary

createStorageLocation in DataflowEnvironment is now a trivial wrapper around the logic in DataflowAnalysisContext.
Additionally, getObjectFields and getFieldsFromClassHierarchy (required for the implementation of createStorageLocation) are also moved to DataflowAnalysisContext.

[sgatev]

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h

91–92

Let's implement this for the new members you are adding and keep the existing members in DataflowEnvironment as they are. I suggest calling the new ones getStableStorageLocation.

94–95

102–104

107–109

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

41–43

I think it makes sense to keep this comment in the wrapper in DataflowEnvironment. Same for the one below.

[wyt]

Address comments.

[sgatev]

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h

91

98

101

[wyt]

Fix comment.

[gribozavr2]

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h

47

[wyt]

Fix comment.

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h

 ///   otherwise omits the node afterwards.
 ///
 ///   * `ExprWithCleanups` - The CFG will generate the appropriate calls to
 ///   destructors and then omit the node.
 ///
 const Expr &ignoreCFGOmittedNodes(const Expr &E);
 const Stmt &ignoreCFGOmittedNodes(const Stmt &S);
 
+/// Gets the set of all fields in the type.
+llvm::DenseSet<const FieldDecl *> getObjectFields(QualType Type);
+
 /// Owns objects that encompass the state of a program and stores context that
 /// is used during dataflow analysis.
 class DataflowAnalysisContext {
 public:
   /// Constructs a dataflow analysis context.
   ///
   /// Requirements:
   ///
public:
   template <typename T>
   typename std::enable_if<std::is_base_of<Value, T>::value, T &>::type
   takeOwnership(std::unique_ptr<T> Val) {
     assert(Val != nullptr);
     Vals.push_back(std::move(Val));
     return *cast<T>(Vals.back().get());
   }
 
+  // FIXME: Rename `createOrGetStorageLocation` to `getOrCreateStorageLocation`,
+  // `getStableStorageLocation`, or something more appropriate.
+
+  /// Creates a storage location appropriate for `Type`. Does not assign a value
+  /// to the returned storage location in the environment.
+  ///
+  /// Requirements:
+  ///
+  ///  `Type` must not be null.
+  StorageLocation &createStorageLocation(QualType Type);
+
+  /// Creates a storage location for `D`. Does not assign the returned storage
+  /// location to `D` in the environment. Does not assign a value to the
+  /// returned storage location in the environment.
+  StorageLocation &createStorageLocation(const VarDecl &D);
+
+  /// Creates a storage location for `E`. Does not assign the returned storage
+  /// location to `E` in the environment. Does not assign a value to the
+  /// returned storage location in the environment.
+  StorageLocation &createStorageLocation(const Expr &E);
+
   /// Assigns `Loc` as the storage location of `D`.
   ///
   /// Requirements:
   ///
   ///  `D` must not be assigned a storage location.
   void setStorageLocation(const ValueDecl &D, StorageLocation &Loc) {
     assert(DeclToLoc.find(&D) == DeclToLoc.end());
     DeclToLoc[&D] = &Loc;

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

 #include "clang/Analysis/FlowSensitive/Value.h"
 #include <cassert>
 #include <memory>
 #include <utility>
 
 namespace clang {
 namespace dataflow {
 
+StorageLocation &DataflowAnalysisContext::createStorageLocation(QualType Type) {
+  assert(!Type.isNull());
+  if (Type->isStructureOrClassType() || Type->isUnionType()) {
+    // FIXME: Explore options to avoid eager initialization of fields as some of
+    // them might not be needed for a particular analysis.
+    llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
+    for (const FieldDecl *Field : getObjectFields(Type))
+      FieldLocs.insert({Field, &createStorageLocation(Field->getType())});
+    return takeOwnership(
+        std::make_unique<AggregateStorageLocation>(Type, std::move(FieldLocs)));
+  }
+  return takeOwnership(std::make_unique<ScalarStorageLocation>(Type));
+}
+
+StorageLocation &
+DataflowAnalysisContext::createStorageLocation(const VarDecl &D) {
+  // Evaluated declarations are always assigned the same storage locations to
+  // ensure that the environment stabilizes across loop iterations. Storage
+  // locations for evaluated declarations are stored in the analysis context.
+  if (auto *Loc = getStorageLocation(D))
+    return *Loc;
+  auto &Loc = createStorageLocation(D.getType());
+  setStorageLocation(D, Loc);
+  return Loc;
+}
+
+StorageLocation &DataflowAnalysisContext::createStorageLocation(const Expr &E) {
+  // Evaluated expressions are always assigned the same storage locations to
+  // ensure that the environment stabilizes across loop iterations. Storage
+  // locations for evaluated expressions are stored in the analysis context.
+  if (auto *Loc = getStorageLocation(E))
+    return *Loc;
+  auto &Loc = createStorageLocation(E.getType());
+  setStorageLocation(E, Loc);
+  return Loc;
+}
+
 static std::pair<BoolValue *, BoolValue *>
 makeCanonicalBoolValuePair(BoolValue &LHS, BoolValue &RHS) {
   auto Res = std::make_pair(&LHS, &RHS);
   if (&RHS < &LHS)
     std::swap(Res.first, Res.second);
   return Res;
 }
 
const Expr &clang::dataflow::ignoreCFGOmittedNodes(const Expr &E) {
   return *Current;
 }
 
 const Stmt &clang::dataflow::ignoreCFGOmittedNodes(const Stmt &S) {
   if (auto *E = dyn_cast<Expr>(&S))
     return ignoreCFGOmittedNodes(*E);
   return S;
 }
+
+// FIXME: Does not precisely handle non-virtual diamond inheritance. A single
+// field decl will be modeled for all instances of the inherited field.
+static void
+getFieldsFromClassHierarchy(QualType Type,
+                            llvm::DenseSet<const FieldDecl *> &Fields) {
+  if (Type->isIncompleteType() || Type->isDependentType() ||
+      !Type->isRecordType())
+    return;
+
+  for (const FieldDecl *Field : Type->getAsRecordDecl()->fields())
+    Fields.insert(Field);
+  if (auto *CXXRecord = Type->getAsCXXRecordDecl())
+    for (const CXXBaseSpecifier &Base : CXXRecord->bases())
+      getFieldsFromClassHierarchy(Base.getType(), Fields);
+}
+
+/// Gets the set of all fields in the type.
+llvm::DenseSet<const FieldDecl *>
+clang::dataflow::getObjectFields(QualType Type) {
+  llvm::DenseSet<const FieldDecl *> Fields;
+  getFieldsFromClassHierarchy(Type, Fields);
+  return Fields;
+}

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

if (auto *DS = dyn_cast<DeclStmt>(&S)) {
     }
   } else if (auto *E = dyn_cast<DeclRefExpr>(&S)) {
     initGlobalVar(*E->getDecl(), Env);
   } else if (auto *E = dyn_cast<MemberExpr>(&S)) {
     initGlobalVar(*E->getMemberDecl(), Env);
   }
 }
 
-// FIXME: Does not precisely handle non-virtual diamond inheritance. A single
-// field decl will be modeled for all instances of the inherited field.
-static void
-getFieldsFromClassHierarchy(QualType Type,
-                            llvm::DenseSet<const FieldDecl *> &Fields) {
-  if (Type->isIncompleteType() || Type->isDependentType() ||
-      !Type->isRecordType())
-    return;
-
-  for (const FieldDecl *Field : Type->getAsRecordDecl()->fields())
-    Fields.insert(Field);
-  if (auto *CXXRecord = Type->getAsCXXRecordDecl())
-    for (const CXXBaseSpecifier &Base : CXXRecord->bases())
-      getFieldsFromClassHierarchy(Base.getType(), Fields);
-}
-
-/// Gets the set of all fields in the type.
-static llvm::DenseSet<const FieldDecl *> getObjectFields(QualType Type) {
-  llvm::DenseSet<const FieldDecl *> Fields;
-  getFieldsFromClassHierarchy(Type, Fields);
-  return Fields;
-}
-
 Environment::Environment(DataflowAnalysisContext &DACtx)
     : DACtx(&DACtx), FlowConditionToken(&DACtx.makeFlowConditionToken()) {}
 
 Environment::Environment(const Environment &Other)
     : DACtx(Other.DACtx), DeclToLoc(Other.DeclToLoc),
       ExprToLoc(Other.ExprToLoc), LocToVal(Other.LocToVal),
       MemberLocToStruct(Other.MemberLocToStruct),
       FlowConditionToken(&DACtx->forkFlowCondition(*Other.FlowConditionToken)) {
if (LocToVal.size() != JoinedEnv.LocToVal.size())
     Effect = LatticeJoinEffect::Changed;
 
   *this = std::move(JoinedEnv);
 
   return Effect;
 }
 
 StorageLocation &Environment::createStorageLocation(QualType Type) {
-  assert(!Type.isNull());
-  if (Type->isStructureOrClassType() || Type->isUnionType()) {
-    // FIXME: Explore options to avoid eager initialization of fields as some of
-    // them might not be needed for a particular analysis.
-    llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
-    for (const FieldDecl *Field : getObjectFields(Type))
-      FieldLocs.insert({Field, &createStorageLocation(Field->getType())});
-    return takeOwnership(
-        std::make_unique<AggregateStorageLocation>(Type, std::move(FieldLocs)));
-  }
-  return takeOwnership(std::make_unique<ScalarStorageLocation>(Type));
+  return DACtx->createStorageLocation(Type);
 }
 
 StorageLocation &Environment::createStorageLocation(const VarDecl &D) {
-  // Evaluated declarations are always assigned the same storage locations to
-  // ensure that the environment stabilizes across loop iterations. Storage
-  // locations for evaluated declarations are stored in the analysis context.
-  if (auto *Loc = DACtx->getStorageLocation(D))
-    return *Loc;
-  auto &Loc = createStorageLocation(D.getType());
-  DACtx->setStorageLocation(D, Loc);
-  return Loc;
+  return DACtx->createStorageLocation(D);
 }
 
 StorageLocation &Environment::createStorageLocation(const Expr &E) {
-  // Evaluated expressions are always assigned the same storage locations to
-  // ensure that the environment stabilizes across loop iterations. Storage
-  // locations for evaluated expressions are stored in the analysis context.
-  if (auto *Loc = DACtx->getStorageLocation(E))
-    return *Loc;
-  auto &Loc = createStorageLocation(E.getType());
-  DACtx->setStorageLocation(E, Loc);
-  return Loc;
+  return DACtx->createStorageLocation(E);
 }
 
 void Environment::setStorageLocation(const ValueDecl &D, StorageLocation &Loc) {
   assert(DeclToLoc.find(&D) == DeclToLoc.end());
   DeclToLoc[&D] = &Loc;
 }
 
 StorageLocation *Environment::getStorageLocation(const ValueDecl &D,