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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
