aaronpuchert added a comment.
Didn't really check for correctness yet, just a superficial review.
I like the idea, splitting the functionality up definitely helps understanding
this.
================
Comment at: clang/include/clang/Sema/Sema.h:4733
+
+ bool isMoveEligible() const { return S >= MoveEligible; };
+ bool isCopyElidable() const { return S == MoveEligibleAndCopyElidable; }
----------------
Nitpick: I'd go with `!= None` here.
================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586
InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
- ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType,
- this->ReturnValue);
+ ExprResult Res =
+ S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
if (Res.isInvalid())
----------------
Perhaps this should just be direct initialization? Can't really find anything
in the standard though.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
if (Ex && !Ex->isTypeDependent()) {
+ NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+
----------------
Any reason why you've moved that away from the comment that wants to explain it?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032
+ Res.S = Res.S != Sema::NRVOResult::None && CanMove
+ ? Sema::NRVOResult::MoveEligible
+ : Sema::NRVOResult::None;
----------------
The way you've designed the enumeration, couldn't you use `std::min` here?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3040
///
-/// \param E The expression being returned from the function or block, or
-/// being thrown.
+/// \\param Parenthesized Whether the expression this candidate was obtained
+/// from was parenthesized.
----------------
The second backslash seems superfluous.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3058
-bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
- CopyElisionSemanticsKind CESK) {
- QualType VDType = VD->getType();
- // - in a return statement in a function with ...
- // ... a class return type ...
- if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
- if (!ReturnType->isRecordType())
- return false;
- // ... the same cv-unqualified type as the function return type ...
- // When considering moving this expression out, allow dissimilar types.
- if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
- !Context.hasSameUnqualifiedType(ReturnType, VDType))
- return false;
+ // (other than a function ... parameter)
+ if (VD->getKind() == Decl::ParmVar) {
----------------
These comments seem to be separated from their context now...
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3059
+ // (other than a function ... parameter)
+ if (VD->getKind() == Decl::ParmVar) {
+ downgradeNRVOResult(Res, hasCXX11);
----------------
Braces are usually omitted if both branches are single statements.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3073
// Return false if VD is a __block variable. We don't want to implicitly move
// out of a __block variable during a return because we cannot assume the
----------------
Just remove "Return false if VD is a __block variable." No need to repeat the
code, what follows is important.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3107
+
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
----------------
Can we maybe move this function up one place to make the diff a bit smaller? It
appears to contain the first part of the original `getCopyElisionCandidate`.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType) {
+ if (!Res.Candidate)
----------------
`NRVOResult` seems to be small, why not make this a proper function and let it
return the result?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+ // If is decltype(auto) and the original return expression
+ // was parethesized, then we can discard this candidate because
+ // it would deduce to a reference.
----------------
parenthesized
================
Comment at: clang/lib/Sema/SemaStmt.cpp:3157-3159
+ // it would deduce to a reference.
+ if (AT->isDecltypeAuto() && Res.isParenthesized)
+ return Res = NRVOResult(), void();
----------------
Can't we just do this when we know what it deduces to? It sounds weird to
handle dependent contexts here because we shouldn't attempt any return value
initialization then.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1062
+ else
+ assert(false && "Unknown context type");
+
----------------
`llvm_unreachable`
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
