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

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

mboehme marked 2 inline comments as done.
mboehme 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
----------------
xazax.hun wrote:
> 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.
> > 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);
> ```

I'm hesitant to do this because even `getOrCreateValue()` is an operation with relatively "loose" semantics. If anything, my sense is that operations like this should go in `TestingSupport.h` rather than `Environment` -- from what I've seen so far, these types of operations tend to occur mostly in tests, while the non-test code usually has more "rigid" semantics.


================
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:
> xazax.hun wrote:
> > 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.
> > > 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);
> > ```
> 
> I'm hesitant to do this because even `getOrCreateValue()` is an operation with relatively "loose" semantics. If anything, my sense is that operations like this should go in `TestingSupport.h` rather than `Environment` -- from what I've seen so far, these types of operations tend to occur mostly in tests, while the non-test code usually has more "rigid" semantics.
> 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.

I'll keep this in mind, but I'll admit I have my doubts. The existing API can be seen as doing this (i.e. providing an API that does not require knowing about value categories), and it is actually more complex to use in many cases, and more error-prone. Fundamentally, I think if you're dealing with static analysis on the Clang AST you need to understand value categories to get the code right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150657