[PATCH] D150657: [clang][dataflow] Use `Strict` accessors in SignAnalysisTest.cpp.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:240
 
+// Returns the `Value` associated with `E` (which may be either a prvalue or
+// glvalue). Creates a `Value` or `StorageLocation` as needed if `E` does not
----------------
mboehme wrote:
> sammccall wrote:
> > this seems to belong on Environment rather than here!
> I've consciously kept this function local to this file for now because it has relatively complex semantics and I haven't seen any other places where these are required so far. I've added a comment noting that this should be moved if it turns out to be needed elsewhere.
> 
> I think that, as much as possible, the methods on `Environment` should be specific about the value category they operate on, because most of the time, it's known what the value category is, and I'd like to encourage users to use the most specific operation possible (because using more general operations breeds bugs).
> I think that, as much as possible, the methods on Environment should be specific about the value category they operate on

I agree, but don't think that's specific to Environment.
I think my favorite factoring of this would be to inline the if/else and move the branches to methods on environment

```
Value *Val = E->isGLValue() ? Env.getOrCreateLValue(*E) : Env.getOrCreateValue(*E);
```

but this is test code, it doesn't matter much, up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150657