[PATCH] D116596: [clang][dataflow] Add transfer functions for assignment

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:36
+/// FIXME: Consider replacing this with a model that is more aligned with C++
+/// value categories.
+enum class SkipPast {
----------------
I'm not sure that value categories are right here.  The key is that skipping is optional, unlike value categories which are fixed.  I think the problem is just that C++ references are weird (for lack of a technical term). A reference variable exists simultaneously in two different categories -- a pointer and the value it points to. That's what this is trying to capture.

So, I'd recommend just changing the name to reflect that. WDYT of something like this?

```
/// Indicates how to treat references, if encountered -- as a reference or the value referred to -- when retrieving
/// storage locations or values.
enum class RefTreatment {
   /// Consider the reference itself.
  AsSelf,
  /// Consider the referent.
  AsReferent,
};
```


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:111
+  /// value in the environment.
+  Value *getValue(const StorageLocation &Loc, SkipPast SP) const;
+
----------------
Why have a SkipPast argument here? I understand the concept for considering var-decls - it corresponds to the weirdness of lvalue-refs themselves. But StorageLocation is a lower-level (and simpler) concept, to which this doesn't seem to map. I'm fine with resolving indirections, but what is the value of only doing so conditionally?


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:242-247
+    if (auto *Val =
+            dyn_cast_or_null<ReferenceValue>(getValue(Loc, SkipPast::None))) {
+      return Val->getPointeeLoc();
+    } else {
+      return Loc;
+    }
----------------



================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:37
+      assert(S->getLHS() != nullptr);
+      const Expr *LHS = S->getLHS()->IgnoreParens();
+      assert(LHS != nullptr);
----------------
maybe comment as to why this is necessary? I'd have thought that CFG's flattening of expressions would handle this (why, in general, we don't need to look deep into expressions).


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:135
+
+    if (const Expr *InitExpr = D.getInit()) {
+      if (D.getType()->isReferenceType()) {
----------------
nit: might be easier to read if you invert this:
```
const Expr *InitExpr = D.getInit();
if (InitExpr == nullptr) return;
```


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:144-145
+        }
+      } else {
+        if (auto *InitExprVal = Env.getValue(*InitExpr, SkipPast::None)) {
+          Env.setValue(Loc, *InitExprVal);
----------------
nit: else-if (one line)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116596