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:
> 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.
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
