nlee updated this revision to Diff 434842.
nlee added a comment.
After some investigation, I think it is not always explicit that removing
`const` makes any improvements when we are "constructing." For example, in this
case <https://godbolt.org/z/5McKK538M>, the two codes which differ in the
return type of function `f` (`S f()` vs. `const S f()`) produce the same
assembly output. I assume this is due to various copy elisions stepping in.
However, when we are "assigning," it makes a difference. In this case
<https://godbolt.org/z/h979nx7ov>, the existence of `const` in the return type
of `f` decides whether the assignment invokes a move or not.
So I'm suggesting a different policy. Warn if:
- It is an assignment expression AND
- RHS is a function call expression AND
- The function returns by-const-value of a class or struct type AND
- The class or struct is move assignable
The following is a part of diagnosed warnings when applied to llvm:
/Users/namgoolee/Documents/llvm-project/llvm/include/llvm/Option/Option.h:103:9:
warning: const qualifying the return value prevents move semantics
[-Wpessimizing-return-by-const]
const Option getGroup() const {
^
/Users/namgoolee/Documents/llvm-project/llvm/lib/Option/ArgList.cpp:38:10:
note: copy assignment is invoked here even if move assignment is supported for
type 'llvm::opt::Option'
O = O.getGroup()) {
^
--
/Users/namgoolee/Documents/llvm-project/clang/include/clang/Lex/MacroInfo.h:421:9:
warning: const qualifying the return value prevents move semantics
[-Wpessimizing-return-by-const]
const DefInfo findDirectiveAtLoc(SourceLocation L,
^
/Users/namgoolee/Documents/llvm-project/clang/include/clang/Lex/Preprocessor.h:1167:10:
note: copy assignment is invoked here even if move assignment is supported for
type 'clang::MacroDirective::DefInfo'
DI = MD->findDirectiveAtLoc(Loc, getSourceManager());
^
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125402/new/
https://reviews.llvm.org/D125402
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaChecking.cpp
clang/lib/Sema/SemaOverload.cpp
clang/test/Misc/warning-wall.c
clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp
Index: clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -fsyntax-only -Wpessimizing-return-by-const -std=c++11 -verify %s
+
+struct S1 {};
+
+S1 f1() {
+ return S1{};
+}
+
+const S1 f1_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+ return S1{};
+}
+
+void run1()
+{
+ S1 s1;
+ s1 = f1();
+ s1 = f1_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'S1'}}
+}
+
+struct S2 {
+ S2() = default;
+ S2(const S2&) = default;
+ S2 &operator=(const S2&) = default;
+ S2 &operator=(S2&&) = default;
+};
+
+S2 f2() {
+ return S2{};
+}
+
+const S2 f2_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+ return S2{};
+}
+
+void run2()
+{
+ S2 s2;
+ s2 = f2();
+ s2 = f2_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'S2'}}
+}
+
+struct S3 {
+ S3() = default;
+ S3(const S3&) = default;
+ S3 &operator=(const S3&) = default;
+};
+
+S3 f3() {
+ return S3{};
+}
+
+const S3 f3_const() { // do not warn if move assignment operator is deleted
+ return S3{};
+}
+
+void run3()
+{
+ S3 s3;
+ s3 = f3();
+ s3 = f3_const();
+}
Index: clang/test/Misc/warning-wall.c
===================================================================
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -33,6 +33,7 @@
CHECK-NEXT: -Wreturn-std-move
CHECK-NEXT: -Wself-move
CHECK-NEXT: -Wmultichar
+CHECK-NEXT: -Wpessimizing-return-by-const
CHECK-NEXT: -Wrange-loop-construct
CHECK-NEXT: -Wreorder
CHECK-NEXT: -Wreorder-ctor
Index: clang/lib/Sema/SemaOverload.cpp
===================================================================
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -13810,9 +13810,10 @@
ArgsArray = ArgsArray.slice(1);
}
- // Check for a self move.
- if (Op == OO_Equal)
+ if (Op == OO_Equal) {
DiagnoseSelfMove(Args[0], Args[1], OpLoc);
+ DiagnosePessimizingConstReturn(Args[1], OpLoc);
+ }
if (ImplicitThis) {
QualType ThisType = Context.getPointerType(ImplicitThis->getType());
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -16800,6 +16800,47 @@
<< RHSExpr->getSourceRange();
}
+void Sema::DiagnosePessimizingConstReturn(const Expr *RHSExpr, SourceLocation OpLoc)
+{
+ if (!getLangOpts().CPlusPlus11 ||
+ inTemplateInstantiation())
+ return;
+
+ RHSExpr = RHSExpr->IgnoreParenImpCasts();
+
+ // Is RHS a function call expression?
+ const CallExpr *CE = dyn_cast<const CallExpr>(RHSExpr);
+ if (!CE)
+ return;
+
+ const Decl *CD = CE->getCalleeDecl();
+ if (!CD)
+ return;
+
+ const FunctionDecl *FD = dyn_cast<const FunctionDecl>(CD);
+ if (!FD)
+ return;
+
+ // Is the return type by-const-value of a struct or class type?
+ const QualType RT = FD->getReturnType();
+ if (!RT->isStructureOrClassType() || !RT.isConstQualified())
+ return;
+
+ // Is the move assignment operator deleted?
+ bool MoveAssignmentIsDeleted = false;
+ for (const CXXMethodDecl *iter : RT->getAsCXXRecordDecl()->methods())
+ if (iter->isMoveAssignmentOperator() && iter->isDeleted())
+ MoveAssignmentIsDeleted = true;
+
+ if (RT->getAsCXXRecordDecl()->hasDefinition() &&
+ RT->getAsCXXRecordDecl()->hasMoveAssignment() &&
+ !MoveAssignmentIsDeleted) {
+ const SourceRange ReturnTypeLoc = FD->getReturnTypeSourceRange();
+ Diag(ReturnTypeLoc.getBegin(), diag::warn_pessimizing_return_by_const);
+ Diag(OpLoc, diag::note_pessimizing_return_by_const) << RT.getUnqualifiedType();
+ }
+}
+
//===--- Layout compatibility ----------------------------------------------//
static bool isLayoutCompatible(ASTContext &C, QualType T1, QualType T2);
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -5070,6 +5070,8 @@
void DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
SourceLocation OpLoc);
+ void DiagnosePessimizingConstReturn(const Expr *RHSExpr, SourceLocation OpLoc);
+
/// Warn if we're implicitly casting from a _Nullable pointer type to a
/// _Nonnull one.
void diagnoseNullableToNonnullConversion(QualType DstType, QualType SrcType,
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6627,6 +6627,13 @@
InGroup<PessimizingMove>, DefaultIgnore;
def note_remove_move : Note<"remove std::move call here">;
+def warn_pessimizing_return_by_const : Warning<
+ "const qualifying the return value prevents move semantics">,
+ InGroup<PessimizingReturnByConst>, DefaultIgnore;
+def note_pessimizing_return_by_const : Note<
+ "copy assignment is invoked here even if move assignment "
+ "is supported for type %0">;
+
def warn_string_plus_int : Warning<
"adding %0 to a string does not append to the string">,
InGroup<StringPlusInt>;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===================================================================
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -555,6 +555,8 @@
def PessimizingMove : DiagGroup<"pessimizing-move">;
def ReturnStdMove : DiagGroup<"return-std-move">;
+def PessimizingReturnByConst : DiagGroup<"pessimizing-return-by-const">;
+
def GNUPointerArith : DiagGroup<"gnu-pointer-arith">;
def PointerArith : DiagGroup<"pointer-arith", [GNUPointerArith]>;
@@ -990,6 +992,7 @@
MissingBraces,
Move,
MultiChar,
+ PessimizingReturnByConst,
RangeLoopConstruct,
Reorder,
ReturnType,
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
