[clang][dataflow][NFC] Eliminate StmtToEnvMap interface.

[mboehme]

Summary

Instead, we turn StmtToEnvMap into a concrete class with the implementation that used to live in StmtToEnvMapImpl.

The layering issue that originally required the indirection through the
StmtToEnvMap interface no longer exists.

[gribozavr2]

clang/include/clang/Analysis/FlowSensitive/Transfer.h

29–31

Could you move the comment to the new declaration of this function?

[mboehme]

Add back comment that I inadvertently deleted.

[mboehme]

clang/include/clang/Analysis/FlowSensitive/Transfer.h

29–31

Oops, done. Thanks for catching!

clang/lib/Analysis/FlowSensitive/Transfer.cpp

144

I'm not changing the return type of getEnvironment() to a reference because https://reviews.llvm.org/D146514 (in review) makes a change that will cause getEnvironment() to return nullptr if S is not reachable, and I want to avoid churn on the return type. (There will be a conflict between the two changes anyway, and I'll deal with that once the first of the changes has landed.)

[mboehme]

After some offline discussion with reviewers, we've come to the conclusion that AnalysisContext isn't a sufficiently clear abstraction to warrant putting it in a header.

For the time being, I'll therefore pursue the less ambitious approach of simply eliminating the StmtToEnvMap interface and instead turning it into a concrete class with the implementation that used to be in StmtToEnvMapImpl.

[mboehme]

Pursue a less ambitious approach, as described in the previous comment.

[mboehme]

@gribozavr2 @xazax.hun Just wanted to confirm that you still regard this as ready to land now that I've changed the patch as we discussed offline?

[gribozavr2]

clang/include/clang/Analysis/FlowSensitive/Transfer.h

35

Drop virtual?

[ymandel]

thanks!

clang/include/clang/Analysis/FlowSensitive/Transfer.h

36–40

Nit: why inline vs putting the definition in Transfer.cpp? I don't have a good sense for the size cutoff on this decision but if it's only used there, then might make sense just to keep the header leaner.

[xazax.hun]

Thanks!

[mboehme]

Rebase to head

[mboehme]

Changes in response to review comments

[mboehme]

clang/include/clang/Analysis/FlowSensitive/Transfer.h

35

Duh. Thanks for catching. Done!

36–40

Good point. Done.

clang/include/clang/Analysis/FlowSensitive/Transfer.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TRANSFER_H
 #define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TRANSFER_H
 
 #include "clang/AST/Stmt.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
 
 namespace clang {
 namespace dataflow {
 
 /// Maps statements to the environments of basic blocks that contain them.
 class StmtToEnvMap {
 public:
-  virtual ~StmtToEnvMap() = default;
-
-  /// Retrieves the environment of the basic block that contains `S`.
-  /// If `S` is reachable, returns a non-null pointer to the environment.
-  /// If `S` is not reachable, returns nullptr.
-  virtual const Environment *getEnvironment(const Stmt &S) const = 0;
+  StmtToEnvMap(const ControlFlowContext &CFCtx,
+               llvm::ArrayRef<std::optional<TypeErasedDataflowAnalysisState>>
+                   BlockToState)
+      : CFCtx(CFCtx), BlockToState(BlockToState) {}
+
+  /// Returns the environment of the basic block that contains `S`.
+  /// The result is guaranteed never to be null.
+  const Environment *getEnvironment(const Stmt &S) const;
+
+private:
+  const ControlFlowContext &CFCtx;
+  llvm::ArrayRef<std::optional<TypeErasedDataflowAnalysisState>> BlockToState;
 };
 
 /// Evaluates `S` and updates `Env` accordingly.
 ///
 /// Requirements:
 ///
 ///  `S` must not be `ParenExpr` or `ExprWithCleanups`.
 void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env);
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TRANSFER_H

clang/lib/Analysis/FlowSensitive/Transfer.cpp

 #include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <memory>
 #include <tuple>
 
 namespace clang {
 namespace dataflow {
 
+const Environment *StmtToEnvMap::getEnvironment(const Stmt &S) const {
+  auto BlockIt = CFCtx.getStmtToBlock().find(&ignoreCFGOmittedNodes(S));
+  assert(BlockIt != CFCtx.getStmtToBlock().end());
+  if (!CFCtx.isBlockReachable(*BlockIt->getSecond()))
+    return nullptr;
+  const auto &State = BlockToState[BlockIt->getSecond()->getBlockID()];
+  assert(State);
+  return &State->Env;
+}
+
 static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
                                           Environment &Env) {
   if (auto *LHSValue =
           dyn_cast_or_null<BoolValue>(Env.getValue(LHS, SkipPast::Reference)))
     if (auto *RHSValue =
             dyn_cast_or_null<BoolValue>(Env.getValue(RHS, SkipPast::Reference)))
       return Env.makeIff(*LHSValue, *RHSValue);
 
static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) {
   if (&UnpackedVal == Val)
     return Val;
   Env.setValue(*Loc, UnpackedVal);
   return &UnpackedVal;
 }
 
 class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 public:
   TransferVisitor(const StmtToEnvMap &StmtToEnv, Environment &Env)
       : StmtToEnv(StmtToEnv), Env(Env) {}
 
   void VisitBinaryOperator(const BinaryOperator *S) {
     const Expr *LHS = S->getLHS();
     assert(LHS != nullptr);
 
     const Expr *RHS = S->getRHS();
     assert(RHS != nullptr);

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 
 #define DEBUG_TYPE "clang-dataflow"
 
 namespace clang {
 namespace dataflow {
 
-class StmtToEnvMapImpl : public StmtToEnvMap {
-public:
-  StmtToEnvMapImpl(
-      const ControlFlowContext &CFCtx,
-      llvm::ArrayRef<std::optional<TypeErasedDataflowAnalysisState>>
-          BlockToState)
-      : CFCtx(CFCtx), BlockToState(BlockToState) {}
-
-  const Environment *getEnvironment(const Stmt &S) const override {
-    auto BlockIt = CFCtx.getStmtToBlock().find(&ignoreCFGOmittedNodes(S));
-    assert(BlockIt != CFCtx.getStmtToBlock().end());
-    if (!CFCtx.isBlockReachable(*BlockIt->getSecond()))
-      return nullptr;
-    const auto &State = BlockToState[BlockIt->getSecond()->getBlockID()];
-    assert(State);
-    return &State->Env;
-  }
-
-private:
-  const ControlFlowContext &CFCtx;
-  llvm::ArrayRef<std::optional<TypeErasedDataflowAnalysisState>> BlockToState;
-};
-
 /// Returns the index of `Block` in the successors of `Pred`.
 static int blockIndexInPredecessor(const CFGBlock &Pred,
                                    const CFGBlock &Block) {
   auto BlockPos = llvm::find_if(
       Pred.succs(), [&Block](const CFGBlock::AdjacentBlock &Succ) {
         return Succ && Succ->getBlockID() == Block.getBlockID();
       });
   return BlockPos - Pred.succ_begin();
for (const CFGBlock *Pred : Preds) {
     const std::optional<TypeErasedDataflowAnalysisState> &MaybePredState =
         AC.BlockStates[Pred->getBlockID()];
     if (!MaybePredState)
       continue;
 
     TypeErasedDataflowAnalysisState PredState = *MaybePredState;
     if (Analysis.builtinOptions()) {
       if (const Stmt *PredTerminatorStmt = Pred->getTerminatorStmt()) {
-        const StmtToEnvMapImpl StmtToEnv(AC.CFCtx, AC.BlockStates);
+        const StmtToEnvMap StmtToEnv(AC.CFCtx, AC.BlockStates);
         auto [Cond, CondValue] =
             TerminatorVisitor(StmtToEnv, PredState.Env,
                               blockIndexInPredecessor(*Pred, Block))
                 .Visit(PredTerminatorStmt);
         if (Cond != nullptr)
           // FIXME: Call transferBranchTypeErased even if BuiltinTransferOpts
           // are not set.
           Analysis.transferBranchTypeErased(CondValue, Cond, PredState.Lattice,

 }
 
 /// Built-in transfer function for `CFGStmt`.
 void builtinTransferStatement(const CFGStmt &Elt,
                               TypeErasedDataflowAnalysisState &InputState,
                               AnalysisContext &AC) {
   const Stmt *S = Elt.getStmt();
   assert(S != nullptr);
-  transfer(StmtToEnvMapImpl(AC.CFCtx, AC.BlockStates), *S, InputState.Env);
+  transfer(StmtToEnvMap(AC.CFCtx, AC.BlockStates), *S, InputState.Env);
 }
 
 /// Built-in transfer function for `CFGInitializer`.
 void builtinTransferInitializer(const CFGInitializer &Elt,
                                 TypeErasedDataflowAnalysisState &InputState) {
   const CXXCtorInitializer *Init = Elt.getInitializer();
   assert(Init != nullptr);