[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99696/new/

https://reviews.llvm.org/D99696