[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

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 =(NT1 &) {
+if (this == )
+  return *this;
+delete[] d;
+d = rhs.d;
+rhs.d = new char[1];
+rhs.d[0] = '\0';
+return *this;
+  }
+
+  NT1(const NT1 ) {
+const int to_alloc = length(rhs.d) + 1;
+d = new char[to_alloc];
+copy(d, rhs.d);
+  }
+
+  NT1 =(const NT1 ) {
+if (this == )
+  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 ) {
+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 ) {
+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 =(NT4 &&) = default;
+
+  NT4(const NT4 &) = default;
+  NT4 =(const NT4 &) = default;
+
+  bool operator==(const NT4 ) {
+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'}}
+}
+
+// 

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

nlee added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6632
+  "const qualifying the return value prevents move semantics">,
+  InGroup, DefaultIgnore;
+def note_pessimizing_return_by_const : Note<

aaron.ballman wrote:
> Given that this is a `DefaultIgnore`, I'm still not quite sure this belongs 
> in Clang rather than continuing to be covered by clang-tidy.
> 
> If you leave it on by default, how many Clang tests break as a result? Do the 
> new warnings all look like true positives? (Basically, I'm trying to see how 
> far away we are from being able to enable this diagnostic by default.)
> how many Clang tests break as a result?
All Clang tests pass without breaking.

> Do the new warnings all look like true positives?
When applied to Clang source code, it does catch some cases as programmed. One 
problem I see is that it also catches for classes with only primitive member 
variables where move assignment(which is copy) has no advantage over copy 
assignment.



Comment at: clang/test/SemaCXX/warn-pessimizing-return-by-const.cpp:50
+
+const S3 f3_const() { // do not warn if move assignment operator is deleted
+  return S3{};

aaron.ballman wrote:
> We should also have a test for an explicitly deleted move assignment, and 
> another one where a member of the class is not movable (and thus the move 
> assignment should be deleted that way as well).
I'll add those cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125402/new/

https://reviews.llvm.org/D125402

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

nlee updated this revision to Diff 435113.
nlee added a comment.

Updated diff to pass clang-format test.


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,58 @@
+// 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 =(const S2 &) = default;
+  S2 =(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 =(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(RHSExpr);
+  if (!CE)
+return;
+
+  const Decl *CD = CE->getCalleeDecl();
+  if (!CD)
+return;
+
+  const FunctionDecl *FD = dyn_cast(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 , 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,
 

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

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 , 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 
, 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 =(const S2&) = default;
+  S2 =(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 =(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 @@

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

nlee added a comment.

Thank you for your reviews, and sorry for the mess in my last comment. I'm new 
to clang/Phabricator and trying to get used to it :)




Comment at: clang/lib/Sema/SemaType.cpp:5203
+  T.isConstQualified() &&
+  T->isStructureOrClassType()) {
+const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();

nlee wrote:
> nlee wrote:
> > erichkeane wrote:
> > > I wonder if this is same concern applies to Unions?  Should this just be 
> > > `T->isRecordType()`?  Should we do more analysis to ensure that this is a 
> > > movable type? I THINK 
> > > `CXXRecordDecl::defaultedMoveConstructorIsDeleted()` should be enough to 
> > > test for t his.
> > How about adding `CXXRecordDecl::hasMoveConstructor()` to ensure the type 
> > is movable? I left out unions because they may alert false positives. An 
> > example of such a case is:
> > ```
> > union U { int i; std::string s; };
> > const U f() { return U{239}; }
> > ```
> > I wonder if this is same concern applies to Unions?  Should this just be 
> > `T->isRecordType()`?  Should we do more analysis to ensure that this is a 
> > movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()` 
> > should be enough to test for t his.
> 
> 
It seems it is not always possible to call 
`T->getAsCXXRecordDecl()->hasMoveConstructor()` here. 
`CXXRecordDecl::DefinitionData` is not populated if definition is not provided 
in the translation unit, such as in the following case:
```
extern struct no_def s;
const no_def f();
```
`assert` fails with message: `"queried property of class with no definition"`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125402/new/

https://reviews.llvm.org/D125402

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

nlee added a comment.

In D125402#3508845 , @aaron.ballman 
wrote:

> Thank you for working on this new diagnostic! We don't typically add new 
> diagnostics that are off by default unless they're for pedantic diagnostics 
> or are reasonably expected to be enabled by default in the future. This is 
> because we've had evidence that suggests such diagnostics aren't enabled 
> often enough to warrant the maintenance cost for them.
>
> I'm not convinced this diagnostic can be enabled by default. Yes, it prevents 
> move semantics, but that's not typically an issue with the correctness of the 
> code. IIRC, we decided to put this diagnostic into clang-tidy because we 
> thought it would be too chatty in practice, especially on older code bases.
>
> Perhaps there are ways we can improve the diagnostic behavior to allow it to 
> be enabled by default though. One possibility would be to look at the return 
> type to see if there's a user-provided move constructor (either `= default` 
> or explicitly written) and only diagnose if one is found. But I think we'd 
> want some evidence that this actually reduces the false positive rate in 
> practice, which means trying to compile some large C++ projects (of various 
> ages/code quality) to see. Would you be willing to run your patch against 
> some large C++ projects to see what kind of true and false positives it 
> generates? From there, we can decide whether we need additional heuristics or 
> not.

How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is 
movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:

  In file included from 
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/base.hpp:662:
  
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/check.hpp:16:12: 
warning: returning by const value prevents move semantics 
[-Wreturn-by-const-value]
  CV_EXPORTS const String typeToString(int type);
 ^~
  
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/check.hpp:26:12: 
warning: returning by const value prevents move semantics 
[-Wreturn-by-const-value]
  CV_EXPORTS const cv::String typeToString_(int type);
 ^~

@aaron.ballman Can you suggest some other projects to test? I'm thinking of 
these: protobuf, pytorch

> I wonder if this is same concern applies to Unions?  Should this just be 
> `T->isRecordType()`?  Should we do more analysis to ensure that this is a 
> movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()` 
> should be enough to test for t his.

@erichkeane Does adding `CXXRecordDecl::hasMoveConstructor()` suffices to 
ensure the type is movable? I left out unions because they may alert false 
positives. An example of such a case is:

  union U { int i; std::string s; };
  const U f() { return U{239}; }




Comment at: clang/lib/Sema/SemaType.cpp:5203
+  T.isConstQualified() &&
+  T->isStructureOrClassType()) {
+const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();

erichkeane wrote:
> I wonder if this is same concern applies to Unions?  Should this just be 
> `T->isRecordType()`?  Should we do more analysis to ensure that this is a 
> movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()` 
> should be enough to test for t his.
How about adding `CXXRecordDecl::hasMoveConstructor()` to ensure the type is 
movable? I left out unions because they may alert false positives. An example 
of such a case is:
```
union U { int i; std::string s; };
const U f() { return U{239}; }
```



Comment at: clang/lib/Sema/SemaType.cpp:5203
+  T.isConstQualified() &&
+  T->isStructureOrClassType()) {
+const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();

nlee wrote:
> erichkeane wrote:
> > I wonder if this is same concern applies to Unions?  Should this just be 
> > `T->isRecordType()`?  Should we do more analysis to ensure that this is a 
> > movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()` 
> > should be enough to test for t his.
> How about adding `CXXRecordDecl::hasMoveConstructor()` to ensure the type is 
> movable? I left out unions because they may alert false positives. An example 
> of such a case is:
> ```
> union U { int i; std::string s; };
> const U f() { return U{239}; }
> ```
> I wonder if this is same concern applies to Unions?  Should this just be 
> `T->isRecordType()`?  Should we do more analysis to ensure that this is a 
> movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()` 
> should be enough to test for t his.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125402/new/

https://reviews.llvm.org/D125402

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Namgoo Lee via Phabricator via cfe-commits]

nlee created this revision.
nlee added a reviewer: lattner.
Herald added a project: All.
nlee requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Returning class or struct by value with const qualifier should be avoided since 
it prevents move semantics.

clang-tidy has readability-const-return-type that catches this case, but it 
also impacts move semantics, not just readability.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125402

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/Misc/warning-wall.c
  clang/test/SemaCXX/warn-return-by-const-value.cpp

Index: clang/test/SemaCXX/warn-return-by-const-value.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-return-by-const-value.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-by-const-value -Wignored-qualifiers -std=c++11 -verify %s
+
+struct S{} s;
+
+const S f1() // expected-warning {{returning by const value prevents move semantics}}
+{
+  return s;
+}
+
+// warn only on top-level const
+const S& f2()
+{
+  return s;
+}
+
+// same as f2()
+const S* f3()
+{
+  return 
+}
+
+// top-level const on primitive type(e.g. pointer) is handled by -Wignored-qualifiers
+S* const f4() // expected-warning {{'const' type qualifier on return type has no effect}}
+{
+  return 
+}
+
+// same as f4()
+const S* const f5() // expected-warning {{'const' type qualifier on return type has no effect}}
+{
+  return 
+}
\ No newline at end of file
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -37,6 +37,7 @@
 CHECK-NEXT:-Wreorder
 CHECK-NEXT:  -Wreorder-ctor
 CHECK-NEXT:  -Wreorder-init-list
+CHECK-NEXT:-Wreturn-by-const-value
 CHECK-NEXT:-Wreturn-type
 CHECK-NEXT:  -Wreturn-type-c-linkage
 CHECK-NEXT:-Wself-assign
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -5197,6 +5197,15 @@
   S.Diag(DeclType.Loc, diag::warn_deprecated_volatile_return) << T;
   }
 
+  // const qualifier on by-value return type prevents move semantics
+  if (S.getLangOpts().CPlusPlus11 &&
+  T.isConstQualified() &&
+  T->isStructureOrClassType()) {
+const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();
+S.Diag(ConstLoc, diag::warn_return_by_const_value)
+<< T << FixItHint::CreateRemoval(ConstLoc);
+  }
+
   // Objective-C ARC ownership qualifiers are ignored on the function
   // return type (by type canonicalization). Complain if this attribute
   // was written here.
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6618,6 +6618,10 @@
   InGroup, DefaultIgnore;
 def note_remove_move : Note<"remove std::move call here">;
 
+def warn_return_by_const_value : Warning<
+  "returning by const value prevents move semantics">,
+  InGroup, DefaultIgnore;
+
 def warn_string_plus_int : Warning<
   "adding %0 to a string does not append to the string">,
   InGroup;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -566,6 +566,7 @@
 def GNURedeclaredEnum : DiagGroup<"gnu-redeclared-enum">;
 def RedundantMove : DiagGroup<"redundant-move">;
 def Register : DiagGroup<"register", [DeprecatedRegister]>;
+def ReturnByConstValue : DiagGroup<"return-by-const-value">;
 def ReturnTypeCLinkage : DiagGroup<"return-type-c-linkage">;
 def ReturnType : DiagGroup<"return-type", [ReturnTypeCLinkage]>;
 def BindToTemporaryCopy : DiagGroup<"bind-to-temporary-copy",
@@ -987,6 +988,7 @@
 MultiChar,
 RangeLoopConstruct,
 Reorder,
+ReturnByConstValue,
 ReturnType,
 SelfAssignment,
 SelfMove,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits