aaronpuchert added inline comments.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3066
- if (isCopyElisionCandidate(ReturnType, VD, CESK))
- return VD;
- return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult &Res, bool CanMove) {
+ Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible
----------------
Quuxplusone wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > mizvekov wrote:
> > > > mizvekov wrote:
> > > > > Quuxplusone wrote:
> > > > > > mizvekov wrote:
> > > > > > > rsmith wrote:
> > > > > > > > I find this name a little unclear (what do we mean by
> > > > > > > > "downgrade"?). Can we call this something a bit more specific,
> > > > > > > > such as `disallowCopyElision` or `disallowNRVO`?
> > > > > > > How about `demoteNRVOStatus`?
> > > > > > I'm with Richard here: if you mean "disallow copy elision," say so!
> > > > > > IMO this should be member functions of `NamedReturnInfo`, and it
> > > > > > should just be
> > > > > >
> > > > > > void disallowCopyElision() {
> > > > > > S &= ~CopyElidable; // obviously this "should" work;
> > > > > > } // ensure that your enum values actually
> > > > > > make it work
> > > > > >
> > > > > > void disallowImplicitMove() {
> > > > > > S = None;
> > > > > > }
> > > > > >
> > > > > > The way you use it below is equivalent to e.g.
> > > > > >
> > > > > > if (VD->isExceptionVariable()) {
> > > > > > Info.disallowCopyElision();
> > > > > > if (!hasCXX20)
> > > > > > Info.disallowImplicitMove();
> > > > > > }
> > > > > >
> > > > > > which I think is a lot easier to puzzle out what's going on and how
> > > > > > it corresponds to the standardese.
> > > > > @Quuxplusone But I did not mean "disallow copy elision", the function
> > > > > will also disallow implicit move based on a parameter, so would be a
> > > > > bit misleading.
> > > > >
> > > > > That solution you proposed though is more repetitive and prone to
> > > > > error: it does not enforce the invariant that you should always
> > > > > disallow copy elision when disallowing implicit move.
> > > > > Even if you amend that to having one manipulator function which
> > > > > enforces the invariant, you still have two bools totaling four
> > > > > states, where only three states make sense.
> > > > >
> > > > > I would consider a better name though, not necessarily happy with
> > > > > this one either.
> > > > Also I did consider making this a method, but this function is more of
> > > > an implementation detail that is better to hide anyway.
> > > You can have more aptly named functions without dropping the invariant.
> > > I'd say you only need `disallowCopyElision` though, instead of
> > > `disallowImplicitMove` you could just directly overwrite with `None`.
> > But the dominant use case here is wanting to choose either one based on the
> > language mode. So the signature taking a bool is more convenient.
> Just to be clear, I don't see the use-case anywhere as wanting to "choose"
> anything based on a parameter. As my code indicated, I see the logic here as
> being simply "For exception variables [or whatever], disallow copy elision.
> _Additionally_, pre-'20, disallow even implicit move for them." And so on. No
> functions of boolean parameters, just straightforward English if-thens, based
> on the English in the standard.
> I get that you're probably not going to write the straightforward code in the
> end; but I just wanted to push a tiny bit harder to see if anyone else sees
> what I see. :)
>
Your argument makes sense to me, but the function is called 5 times with a
runtime parameter, so it's going to be quite a bit more code.
> just straightforward English if-thens, based on the English in the standard.
Technically aren't we handling different standards at once here? So there is
actually no language in any standard that contains these particular if-thens.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99696/new/
https://reviews.llvm.org/D99696
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
