[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.

Really nice patch. Thanks for this improvement!



================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:307-311
+      auto &ParamLoc =
+          createStorageLocation(ParamDecl->getType().getNonReferenceType());
       setStorageLocation(*ParamDecl, ParamLoc);
-      if (Value *ParamVal = createValue(ParamDecl->getType()))
-        setValue(ParamLoc, *ParamVal);
+      if (Value *ParamVal =
+              createValue(ParamDecl->getType().getNonReferenceType()))
----------------
Maybe comment as to why we're not use the `VarDecl` overloads of `createStorageLocation` and why we're specifically using `getNonReferenceType` for `createValue`.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:610
   assert(!DeclToLoc.contains(&D));
+  assert(dyn_cast_or_null<ReferenceValue>(getValue(Loc)) == nullptr);
   DeclToLoc[&D] = &Loc;
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:631
+
+  assert(dyn_cast_or_null<ReferenceValue>(getValue(*Loc)) == nullptr);
+
----------------
would `isa_and_nonnull` work?


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:279-281
+      if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
+        Env.setValue(Loc, *InitExprVal);
+      }
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:329
+          // against this above.
+          ProcessVarDecl(*VD);
+          auto *VDLoc = Env.getStorageLocation(*VD);
----------------
why the recursive call rather than relying on what we know about their structure?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149144/new/

https://reviews.llvm.org/D149144