[PATCH] D150655: [clang][dataflow] Use `Strict` accessors in more places in Transfer.cpp.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:138
 
+static void forwardValue(const Expr &From, const Expr &To, Environment &Env) {
+  if (auto *Val = Env.getValueStrict(From))
----------------
mboehme wrote:
> sammccall wrote:
> > mboehme wrote:
> > > ymandel wrote:
> > > > mboehme wrote:
> > > > > sammccall wrote:
> > > > > > the name "forward" isn't clear to me - if anything suggesting std::forward, but not assignment.
> > > > > > 
> > > > > > Maybe `assignValue` or `copyValue`, and `aliasLocation` for the storageloc variant?
> > > > > > 
> > > > > > That leaves "forwardValueOrStorageLocation", but that only has one callsite and part of the point of this refactoring is that we're best thinking about these cases explicitly, right? So could just inline the if.
> > > > > (Haven't made any changes yet because I'd like to discuss first.)
> > > > > 
> > > > > > That leaves "forwardValueOrStorageLocation", but that only has one callsite and part of the point of this refactoring is that we're best thinking about these cases explicitly, right? So could just inline the if.
> > > > > 
> > > > > This function will end up being used in more places though. I have some draft patches that use it (for the comma operator, for example) but these aren't ready for review yet. But as I know the operation will be used elsewhere, it made sense to create an abstraction now (and we'll definitely need a name for it at some point).
> > > > > 
> > > > > I think the most generic of the names you suggest is `copy`, and I think it works reasonably well: `copyValue()`, `copyStorageLocation()`, `copyValueOrStorageLocation()`. WDYT?
> > > > I see Martin's point in not wanting to use copy/assign, since the same value will be shared between the locations. but, I see why "forward" has multiple meanings. Maybe "share" or "propogate"?
> > > I like "propagate", since it conveys the same meaning that I was going for with "forward", but without the allusion to `std::forward`. I like it better than "copy" or "assign" because those also have specific meanings in C++, and "propagate" doesn't.
> > > 
> > > @sammccall WDYT?
> > Propagate is fine, personally I like "share" better though because "sharing a location" is direct and "propagating a location" is abstract.
> > 
> > I don't think `propagateValueOrStorageLocation()` should exist though. I can deal with the new model where lvalues and rvalues are totally different things, or the old model where we pretend references are bizarro-pointers and lvalues and rvalues are basically the same. I don't think saving a few if statements is a good reason to mix the two. In the new model the function does two ~unrelated things.
> > Propagate is fine, personally I like "share" better though because "sharing a location" is direct and "propagating a location" is abstract.
> 
> "Share" doesn't imply a direction ("from ... to ...") in the way that "propagate" does -- so if it's OK with you, I'd like to use "propagate".
> 
> > I don't think `propagateValueOrStorageLocation()` should exist though. I can deal with the new model where lvalues and rvalues are totally different things, or the old model where we pretend references are bizarro-pointers and lvalues and rvalues are basically the same. I don't think saving a few if statements is a good reason to mix the two. In the new model the function does two ~unrelated things.
> 
> While most AST nodes operate either on prvalues or glvalues, there are some that can propagate either a prvalue or a glvalue. For example:
> 
>   - The conditional operator
>   - The comma operator (for the RHS)
>   - No-op casts
> 
> The operation I'm trying to express is "propagate a value of any category (but don't change its category)". I think this is a useful abstraction that corresponds to a concept in the language, but I'm happy to inline the code if you feel strongly about this.
Yeah, propagate is fine, and go ahead with the function, I don't think I feel strongly about it in the end.

> propagate a value of any category

(For the record, I think "value of any category" is pretty hard to keep hold of now: it has no type (`Value` and `StorageLocation` are distinct) and no name (we're using "Value" to mean ~rvalue), which is why I find it hard to think about this operation)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150655