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