nlee updated this revision to Diff 437011.
nlee added a comment.
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 has non-trivial move assignment operator
Removed `default-ignore` flag and `make check-clang` passes.
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,293 @@
+// RUN: %clang_cc1 -fsyntax-only -Wpessimizing-return-by-const -std=c++11 -verify %s
+
+// A class with non-trivial move assignment operator
+struct NT1 {
+ char *d;
+ NT1() {
+ d = new char[1];
+ d[0] = '\0';
+ }
+
+ NT1(const char *c_str) {
+ const int to_alloc = length(c_str) + 1;
+ d = new char[to_alloc];
+ copy(d, c_str);
+ }
+
+ ~NT1() {
+ delete[] d;
+ }
+
+ NT1 &operator=(NT1 &&rhs) {
+ if (this == &rhs)
+ return *this;
+ delete[] d;
+ d = rhs.d;
+ rhs.d = new char[1];
+ rhs.d[0] = '\0';
+ return *this;
+ }
+
+ NT1(const NT1 &rhs) {
+ const int to_alloc = length(rhs.d) + 1;
+ d = new char[to_alloc];
+ copy(d, rhs.d);
+ }
+
+ NT1 &operator=(const NT1 &rhs) {
+ if (this == &rhs)
+ return *this;
+ delete[] d;
+ const int to_alloc = length(rhs.d) + 1;
+ d = new char[to_alloc];
+ copy(d, rhs.d);
+ return *this;
+ }
+
+ // c_str : null-terminated c string. If empty, c_str[0] is '\0'. Cannot be nullptr.
+ // Returns the length of the string, excluding the null-terminating byte.
+ // If given an empty string, returns 0.
+ static int length(const char *c_str) {
+ int ret = 0;
+ while (*c_str++)
+ ++ret;
+ return ret;
+ }
+
+ // dst : must be pre-allocated
+ // Copies src to dst, including the null-terminator.
+ static void copy(char *dst, const char *src) {
+ while ((*dst++ = *src++))
+ ;
+ }
+
+ bool operator==(const NT1 &rhs) {
+ if (length(d) != length(rhs.d))
+ return false;
+ int cnt = 0;
+ const char *s1 = d;
+ const char *s2 = rhs.d;
+ while (cnt++ < length(d))
+ if (*s1++ != *s2++)
+ return false;
+ return true;
+ }
+};
+
+NT1 f_nt1() {
+ return NT1{"f_nt1"};
+}
+
+const NT1 f_nt1_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+ return NT1{"f_nt1_const"};
+}
+
+void run_nt1() {
+ NT1 nt1;
+ nt1 = f_nt1();
+ nt1 = f_nt1_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT1'}}
+}
+
+// NT2 has non-trivial move assignment operator since the move assignment operator selected
+// to move the base class NT1 is non-trivial
+struct NT2 : public NT1 {
+ NT2() : NT1() {}
+ NT2(const char *c_str) : NT1(c_str) {}
+};
+
+NT2 f_nt2() {
+ return NT2{"f_nt2"};
+}
+
+const NT2 f_nt2_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+ return NT2{"f_nt2_const"};
+}
+
+void run_nt2() {
+ NT2 nt2;
+ nt2 = f_nt2();
+ nt2 = f_nt2_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT2'}}
+}
+
+// NT3 has non-trivial move assignment operator since the non-static data member of NT1
+// has non-trivial move assignment operator
+struct NT3 {
+ NT1 nt1;
+ NT3() : nt1(NT1{}) {}
+ NT3(const char *c_str) : nt1(NT1(c_str)) {}
+
+ bool operator==(const NT3 &rhs) {
+ return nt1 == rhs.nt1;
+ }
+};
+
+NT3 f_nt3() {
+ return NT3{"f_nt3"};
+}
+
+const NT3 f_nt3_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+ return NT3{"f_nt3_const"};
+}
+
+void run_nt3() {
+ NT3 nt3;
+ nt3 = f_nt3();
+ nt3 = f_nt3_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT3'}}
+}
+
+// NT4 has defaulted move assignment operator which is still non-trivial
+struct NT4 {
+ NT1 nt1;
+ NT4() : nt1(NT1{}) {}
+ NT4(const char *c_str) : nt1(NT1(c_str)) {}
+
+ NT4 &operator=(NT4 &&) = default;
+
+ NT4(const NT4 &) = default;
+ NT4 &operator=(const NT4 &) = default;
+
+ bool operator==(const NT4 &rhs) {
+ return nt1 == rhs.nt1;
+ }
+};
+
+NT4 f_nt4() {
+ return NT4{"f_nt4"};
+}
+
+const NT4 f_nt4_const() { // expected-warning{{const qualifying the return value prevents move semantics}}
+ return NT4{"f_nt4_const"};
+}
+
+void run_nt4() {
+ NT4 nt4;
+ nt4 = f_nt4();
+ nt4 = f_nt4_const(); // expected-note{{copy assignment is invoked here even if move assignment is supported for type 'NT4'}}
+}
+
+// NT5 have deleted move assignment operator. We do not warn in this case
+struct NT5 {
+ NT1 nt1;
+ NT5() : nt1(NT1{}) {}
+ NT5(const char *c_str) : nt1(NT1(c_str)) {}
+
+ NT5 &operator=(NT5 &&) = delete;
+
+ NT5(const NT5 &) = default;
+ NT5 &operator=(const NT5 &) = default;
+
+ bool operator==(const NT5 &rhs) {
+ return nt1 == rhs.nt1;
+ }
+};
+
+const NT5 f_nt5_const() {
+ return NT5{"f_nt5_const"};
+}
+
+void run_nt5() {
+ NT5 nt5;
+ nt5 = f_nt5_const();
+}
+
+// NT6 has deleted move assignment operator because its member(NT5) has deleted move assignment operator.
+// We do not warn in this case.
+struct NT6 {
+ NT5 nt5;
+};
+
+const NT6 f_nt6_const() {
+ return NT6{"f_nt6_const"};
+}
+
+void run_nt6() {
+ NT6 nt6;
+ nt6 = f_nt6_const();
+}
+
+// warning is suppressed for trivial move assignment operator
+
+struct T1 {
+ int i1, i2, i3, i4;
+};
+
+const T1 f_t1_const() {
+ return T1{};
+}
+
+void run_t1() {
+ T1 t1;
+ t1 = f_t1_const();
+}
+
+struct T2 {
+ T2() = default;
+ T2(const T2 &) = default;
+ T2 &operator=(const T2 &) = default;
+ T2 &operator=(T2 &&) = default;
+};
+
+const T2 f_t2_const() {
+ return T2{};
+}
+
+void run_t2() {
+ T2 t2;
+ t2 = f_t2_const();
+}
+
+struct T3 {
+ T3() = default;
+ T3(const T3 &) = default;
+ T3 &operator=(const T3 &) = default;
+};
+
+const T3 f_t3_const() {
+ return T3{};
+}
+
+void run_t3() {
+ T3 t3;
+ t3 = f_t3_const();
+}
+
+struct T4 {
+ T4() = default;
+ T4(const T4 &) = default;
+ T4 &operator=(const T4 &) = default;
+ T4 &operator=(T4 &&) = delete;
+};
+
+const T4 f_t4_const() {
+ return T4{};
+}
+
+void run_t4() {
+ T4 t4;
+ t4 = f_t4_const();
+}
+
+struct T5 {
+ T1 t1;
+};
+
+const T5 f_t5_const() {
+ return T5{};
+}
+
+void run_t5() {
+ T5 t5;
+ t5 = f_t5_const();
+}
+
+struct T6 : public T1 {
+};
+
+const T6 f_t6_const() {
+ return T6{};
+}
+
+void run_t6() {
+ T6 t6;
+ t6 = f_t6_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
@@ -16834,6 +16834,49 @@
<< RHSExpr->getSourceRange();
}
+void Sema::DiagnosePessimizingConstReturn(const Expr *RHSExpr,
+ SourceLocation OpLoc) {
+
+ if (!getLangOpts().CPlusPlus11 || inTemplateInstantiation())
+ return;
+
+ RHSExpr = RHSExpr->IgnoreImplicit();
+
+ // 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 explicitly 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() &&
+ RT->getAsCXXRecordDecl()->hasNonTrivialMoveAssignment() &&
+ !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,9 @@
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>;
+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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
