rsmith added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:4728
- enum CopyElisionSemanticsKind {
- CES_Strict = 0,
- CES_AllowParameters = 1,
- CES_AllowDifferentTypes = 2,
- CES_AllowExceptionVariables = 4,
- CES_AllowRValueReferenceType = 8,
- CES_ImplicitlyMovableCXX11CXX14CXX17 =
- (CES_AllowParameters | CES_AllowDifferentTypes),
- CES_ImplicitlyMovableCXX20 =
- (CES_AllowParameters | CES_AllowDifferentTypes |
- CES_AllowExceptionVariables | CES_AllowRValueReferenceType),
+ struct NRVOResult {
+ const VarDecl *Candidate;
----------------
We generally use `FooResult` to mean "either a `Foo` or an indicator that an
error has been diagnosed". Would the name `NRVOInfo` work instead?
Actually, I think the "O" here is misleading, because we're not talking about
performing the optimization here, just about what variable was named in a
return statement. And I think calling this "NRVO" is a bit misleading because
it also covers implicit move, which isn't traditionally part of NRVO. So how
about `NamedReturnValueInfo` or `NamedReturnInfo` or `NamedReturnValue` or
`ReturnValueName` or something like that?
================
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
----------------
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`?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3138
+/// copy-elidable statuses, considering the function return type criteria
+/// as applicable to return statements.
+///
----------------
This comment needs re-flowing.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3144
+/// \returns The copy elision candidate, in case the initial return expression
+/// was copy elisible, or nullptr otherwise.
+const VarDecl *Sema::getCopyElisionCandidate(NRVOResult &Res,
----------------
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3150-3153
+ // If we got a non-deduced auto ReturnType, we are in a dependent context and
+ // there is no point in allowing copy elision since we won't have it deduced
+ // by the point the VardDecl is instantiated, which is the last chance we
have
+ // of deciding if the candidate is really copy elisible.
----------------
How does this happen? Are there any cases where we could do NRVO or should do
an implicit move that are blocked by this?
It seems to me that we should (nearly always) be able to work out whether copy
elision is possible here without knowing the deduced type:
-- if the return type is //cv// `auto` then it will always be deduced to the
type of the returned variable, so we can always perform copy elision
-- if the return type is `decltype(auto)`, then we can perform copy elision if
the expression is unparenthesized and otherwise cannot; we could perhaps track
whether the expression was parenthesized in `NRVOResult`, and can
conservatively disallow copy elision if we don't know (eg, from template
instantiation, where we're only looking at the variable and not the return
statements)
-- if the return type is anything else involving `auto`, it can't possibly
instantiate to a class type, so we'll never perform copy elision
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3152-3153
+ // there is no point in allowing copy elision since we won't have it deduced
+ // by the point the VardDecl is instantiated, which is the last chance we
have
+ // of deciding if the candidate is really copy elisible.
+ if (AutoType *AT = ReturnType->getContainedAutoType())
----------------
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+ if (AT->isCanonicalUnqualified())
+ return Res = NRVOResult(), nullptr;
+
----------------
Please add braces rather than using a comma expression here and below.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3290
}
-
- if (!getLangOpts().CPlusPlus20 && NeedSecondOverloadResolution &&
- !getDiagnostics().isIgnored(diag::warn_return_std_move,
+ if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
Value->getExprLoc())) {
----------------
Should we skip this if `NRVORes.isMoveEligible() && getLangOpts().CPlusPlus20`?
In that case, we already tried an unrestricted move a couple of lines ago, so
we know that adding `std::move` won't help. (I think we can still hit the
"adding `std::move` will help" case in C++20, but only in obscure corners such
as a `volatile`-qualified local variable.)
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3470
+ InitializedEntity Entity = InitializedEntity::InitializeResult(
+ ReturnLoc, FnRetType, !!NRVOCandidate);
+ ExprResult Res =
----------------
I find the pre-existing `NRVOCandidate != nullptr` more readable than this
`!!NRVOCandidate`.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1058
+ else if (auto *F = dyn_cast<BlockDecl>(DC))
+ // FIXME: get the return type here somehow...
+ ReturnType = SemaRef.Context.getAutoDeductType();
----------------
Can you use `getCurBlock()->FunctionType` for this? We won't have a `Scope`,
but we should still have a `ScopeInfo`.
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
