mboehme marked an inline comment as done.
mboehme 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))
----------------
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.
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
