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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696