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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp:134
                     LatticeTransferState &State) {
-  StorageLocation *Loc = State.Env.getStorageLocation(*BO, SkipPast::None);
-  if (!Loc) {
-    Loc = &State.Env.createStorageLocation(*BO);
-    State.Env.setStorageLocation(*BO, *Loc);
-  }
-  BoolValue *Comp = cast_or_null<BoolValue>(State.Env.getValue(*Loc));
+  BoolValue *Comp = cast_or_null<BoolValue>(State.Env.getValueStrict(*BO));
   if (!Comp) {
----------------
Wow, this is so much cleaner than before!


================
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
----------------
sammccall wrote:
> 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.
So, we could either make an API that forces users to think about value categories explicitly, or we could try to design an API where the users do not need to know about value categories at all. I think it might be worth exploring if the latter is possible without any major caveats, the simpler the APIs are the better. But this is for the future, not for this PR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150657