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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
