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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3144-3145
+/// \param ReturnType This is the return type of the function.
+/// \returns The copy elision candidate, in case the initial return expression
+/// was copy elidable, or nullptr otherwise.
+const VarDecl *Sema::getCopyElisionCandidate(NamedReturnInfo &Info,
----------------
So the return value is trivially deducible from the post-state of `Info` (as `Info.isCopyElidable() ? Info.Candidate : nullptr`)? I guess it makes some code shorter to write, but generally it's preferable to have fewer implicit invariants.

Also it's a getter function that modifies its arguments... not saying there isn't precedent in the code, but I'd say less surprising function signatures are still better where we can have them.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3084-3085
+                                               bool ForceCXX20) {
+  bool hasCXX11 = getLangOpts().CPlusPlus11 || ForceCXX20,
+       hasCXX20 = getLangOpts().CPlusPlus20 || ForceCXX20;
+  NamedReturnInfo Info{VD, NamedReturnInfo::MoveEligibleAndCopyElidable};
----------------
Quuxplusone wrote:
> Nit: please declare variables one-per-line. Change that `,` to a `;` and write `bool` again on line 3085 instead of four space characters.
I don't think the coding standards have a rule about this. I tend to advocate the opposite in most cases to avoid repetition, but for `bool` it's probably fine to repeat.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696