[clang][dataflow] Use `Strict` accessors in TypeErasedDataflowAnalysis.cpp.

[mboehme]

Summary

This patch is part of the ongoing migration to strict handling of value categories (see https://discourse.llvm.org/t/70086 for details).

Depends On D150655

[ymandel]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

310–317

Nit.

[xazax.hun]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

303

I am wondering whether it is more robust to branch directly on the value category of InitStmt, although I cannot think of cases where this condition would go wrong. Feel free to ignore.

[mboehme]

Applied suggested edit.

[mboehme]

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

303

I think it's actually more robust to branch on isReferenceType(), because getStorageLocationStrict contains an assertion that InitStmt is a value category -- so we're verifying that both of these conditions are met. If instead we branched on InitStmt->isGLValue(), we wouldn't be looking at Member->getType()->isReferenceType() at all, and we wouldn't be verifying that it matches our expectations (i.e. that it's a reference).

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

VisitConditionalOperator(const ConditionalOperator *S) {
     auto *Cond = S->getCond();
     assert(Cond != nullptr);
     return extendFlowCondition(*Cond);
   }
 
 private:
   TerminatorVisitorRetTy extendFlowCondition(const Expr &Cond) {
     // The terminator sub-expression might not be evaluated.
-    if (Env.getStorageLocation(Cond, SkipPast::None) == nullptr)
+    if (Env.getValueStrict(Cond) == nullptr)
       transfer(StmtToEnv, Cond, Env);
 
-    // FIXME: The flow condition must be an r-value, so `SkipPast::None` should
-    // suffice.
-    auto *Val =
-        cast_or_null<BoolValue>(Env.getValue(Cond, SkipPast::Reference));
+    auto *Val = cast_or_null<BoolValue>(Env.getValueStrict(Cond));
     // Value merging depends on flow conditions from different environments
     // being mutually exclusive -- that is, they cannot both be true in their
     // entirety (even if they may share some clauses). So, we need *some* value
     // for the condition expression, even if just an atom.
     if (Val == nullptr) {
       Val = &Env.makeAtomicBoolValue();
       Env.setValueStrict(Cond, *Val);
     }
builtinTransferInitializer(const CFGInitializer &Elt,
   const FieldDecl *Member = Init->getMember();
   if (Member == nullptr)
     // Not a field initializer.
     return;
 
   auto *InitStmt = Init->getInit();
   assert(InitStmt != nullptr);
 
-  auto *InitStmtLoc = Env.getStorageLocation(*InitStmt, SkipPast::Reference);
+  if (Member->getType()->isReferenceType()) {
+    auto *InitStmtLoc = Env.getStorageLocationStrict(*InitStmt);
     if (InitStmtLoc == nullptr)
       return;
 
-  auto *InitStmtVal = Env.getValue(*InitStmtLoc);
-  if (InitStmtVal == nullptr)
-    return;
-
-  if (Member->getType()->isReferenceType()) {
     auto &MemberLoc = ThisLoc.getChild(*Member);
     Env.setValue(MemberLoc, Env.create<ReferenceValue>(*InitStmtLoc));
   } else {
+    auto *InitStmtVal = Env.getValueStrict(*InitStmt);
+    if (InitStmtVal == nullptr)
+      return;
+
     auto &MemberLoc = ThisLoc.getChild(*Member);
     Env.setValue(MemberLoc, *InitStmtVal);
   }
 }
 
 static void builtinTransfer(const CFGElement &Elt,
                             TypeErasedDataflowAnalysisState &State,
                             AnalysisContext &AC) {
   switch (Elt.getKind()) {
   case CFGElement::Statement:
     builtinTransferStatement(Elt.castAs<CFGStmt>(), State, AC);