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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
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};
----------------
Nit: please declare variables one-per-line. Change that `,` to a `;` and write `bool` again on line 3085 instead of four space characters.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696