[PATCH] D156672: [clang][dataflow] Use `Strict` accessors where we weren't using them yet.

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 2 inline comments as done.
mboehme added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:545
+          &cast<AggregateStorageLocation>(State.Env.createStorageLocation(*E));
+      State.Env.setStorageLocationStrict(*E, *Loc);
+    }
----------------
ymandel wrote:
> Should this line instead be outside of this `if`? So that the flow is
> ```
> Loc = get
> if (!Loc) Loc = create
> set(Loc)
> ```
I don't understand -- wouldn't this just be doing unnecessary work?

If

```
Loc = cast_or_null<AggregateStorageLocation>(
        State.Env.getStorageLocationStrict(*E));
```

returns a non-null `Loc`, we don't create a new `Loc`, hence

```
State.Env.setStorageLocationStrict(*E, *Loc);
```

is a no-op, as `E` is already associated with `Loc`.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:473-474
       if (S->isElidable()) {
-        Env.setStorageLocation(*S, *ArgLoc);
-      } else if (auto *ArgVal = cast_or_null<StructValue>(Env.getValue(*Arg))) {
+        if (Value *Val = Env.getValue(*ArgLoc))
+          Env.setValueStrict(*S, *Val);
+      } else {
----------------
ymandel wrote:
> What happens otherwise? I gather that `S` just isn't associated with anything?
Correct. The idea being that if `S` is elidable, i.e. just a pass-through, we want to pass through any `StructValue` that we got, and so if we didn't get a `StructValue`, then we shouldn't pass one through.

I guess we could also just create a `StructValue` from scratch and associate it with `ArgLoc`, but I'm not sure that it makes much of a difference in practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156672