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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

This has been sitting in the queue for a while, sorry about that.
I think this makes sense in its current iteration, with the warning always on. 
Have you tried to build a large project like chrome with it?




Comment at: clang/lib/Sema/SemaChecking.cpp:16843
+
+  RHSExpr = RHSExpr->IgnoreImplicit();
+

Maybe we should ignore parentheses too?



Comment at: clang/lib/Sema/SemaChecking.cpp:16850-16856
+  const Decl *CD = CE->getCalleeDecl();
+  if (!CD)
+return;
+
+  const FunctionDecl *FD = dyn_cast(CD);
+  if (!FD)
+return;

you can use `CE->getDirectCallee()` here



Comment at: clang/lib/Sema/SemaChecking.cpp:16874
+const SourceRange ReturnTypeLoc = FD->getReturnTypeSourceRange();
+Diag(ReturnTypeLoc.getBegin(), diag::warn_pessimizing_return_by_const);
+Diag(OpLoc, diag::note_pessimizing_return_by_const)

I think a fixit to remove the const would be nice.


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 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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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<

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.)



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{};

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).


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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D125402#3553855 , @erichkeane 
wrote:

> In D125402#3553841 , @aaron.ballman 
> wrote:
>
>> In D125402#3553825 , @erichkeane 
>> wrote:
>>
>>> In D125402#3553802 , 
>>> @aaron.ballman wrote:
>>>
 In D125402#3517865 , @nlee wrote:

> 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:

 I don't think that will be quite correct -- IIRC, that would still return 
 true if the move constructor was deleted. `hasSimpleMoveConstructor()` and 
 `hasSimpleMoveAssignment()` might be a better approach.
>>>
>>> The 'Simple' version might not be quite right... That is implemented as:
>>>
>>>   bool hasSimpleMoveConstructor() const {
>>>  return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
>>> !data().DefaultedMoveConstructorIsDeleted;
>>>}
>>>
>>>
>>> So this would still warn about user-defined move constructors.
>>
>> Good catch!
>>
>>> HOWEVER, I might suggest `hasMoveConstructor() && 
>>> !defaultedMoveConstructorIsDeleted()` for the ctor test.  There is similar 
>>> storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, 
>>> so you might need to add a function to expose it in DeclCXX.h.
>>
>> I think that might still not be correct because there could be a 
>> user-provided move constructor that's explicitly deleted. (So it has a move 
>> constructor, but the defaulted one is not deleted because it's user 
>> provided.)
>
> I don't think that is possible?  I think 'defaultedMoveConstructor' includes 
> the user typing `StructName(StructName&&) = delete;`.
>
> Note that out of line deleted implementations are prohibited: 
> https://godbolt.org/z/MTK4jGa57 So we don't have to worry about that.

I might be wrong about this above:
https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html#a8ddbc71ffd71a6f7d580fc7a19452f30
Comment says: " 
/// Set that we attempted to declare an implicit move

/// constructor, but overload resolution failed so we deleted it."

So perhaps more work to figure out non-deleted move ctor, it might require a 
lookup.  So if we do that, we need a bunch of tests to validate all these 
conditions I believe (manually deleted, inherited deleted, 
member-cauesd-deleted, and 'defaulted' versions of the last 2, user provided 
but works, etc).


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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D125402#3553841 , @aaron.ballman 
wrote:

> In D125402#3553825 , @erichkeane 
> wrote:
>
>> In D125402#3553802 , 
>> @aaron.ballman wrote:
>>
>>> In D125402#3517865 , @nlee wrote:
>>>
 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:
>>>
>>> I don't think that will be quite correct -- IIRC, that would still return 
>>> true if the move constructor was deleted. `hasSimpleMoveConstructor()` and 
>>> `hasSimpleMoveAssignment()` might be a better approach.
>>
>> The 'Simple' version might not be quite right... That is implemented as:
>>
>>   bool hasSimpleMoveConstructor() const {
>>  return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
>> !data().DefaultedMoveConstructorIsDeleted;
>>}
>>
>>
>> So this would still warn about user-defined move constructors.
>
> Good catch!
>
>> HOWEVER, I might suggest `hasMoveConstructor() && 
>> !defaultedMoveConstructorIsDeleted()` for the ctor test.  There is similar 
>> storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so 
>> you might need to add a function to expose it in DeclCXX.h.
>
> I think that might still not be correct because there could be a 
> user-provided move constructor that's explicitly deleted. (So it has a move 
> constructor, but the defaulted one is not deleted because it's user provided.)

I don't think that is possible?  I think 'defaultedMoveConstructor' includes 
the user typing `StructName(StructName&&) = delete;`.

Note that out of line deleted implementations are prohibited: 
https://godbolt.org/z/MTK4jGa57 So we don't have to worry about that.


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D125402#3553825 , @erichkeane 
wrote:

> In D125402#3553802 , @aaron.ballman 
> wrote:
>
>> In D125402#3517865 , @nlee wrote:
>>
>>> 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:
>>
>> I don't think that will be quite correct -- IIRC, that would still return 
>> true if the move constructor was deleted. `hasSimpleMoveConstructor()` and 
>> `hasSimpleMoveAssignment()` might be a better approach.
>
> The 'Simple' version might not be quite right... That is implemented as:
>
>   bool hasSimpleMoveConstructor() const {
>  return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
> !data().DefaultedMoveConstructorIsDeleted;
>}
>
>
> So this would still warn about user-defined move constructors.

Good catch!

> HOWEVER, I might suggest `hasMoveConstructor() && 
> !defaultedMoveConstructorIsDeleted()` for the ctor test.  There is similar 
> storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so 
> you might need to add a function to expose it in DeclCXX.h.

I think that might still not be correct because there could be a user-provided 
move constructor that's explicitly deleted. (So it has a move constructor, but 
the defaulted one is not deleted because it's user provided.)


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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D125402#3553802 , @aaron.ballman 
wrote:

> In D125402#3517865 , @nlee wrote:
>
>> 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:
>
> I don't think that will be quite correct -- IIRC, that would still return 
> true if the move constructor was deleted. `hasSimpleMoveConstructor()` and 
> `hasSimpleMoveAssignment()` might be a better approach.

The 'Simple' version might not be quite right... That is implemented as:

  bool hasSimpleMoveConstructor() const {
 return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
!data().DefaultedMoveConstructorIsDeleted;
   }


So this would still warn about user-defined move constructors.

HOWEVER, I might suggest `hasMoveConstructor() && 
!defaultedMoveConstructorIsDeleted()` for the ctor test.  There is similar 
storage for the 'DefaultedMoveAssignmentIsDeleted`, but it isn't exposed, so 
you might need to add a function to expose it in DeclCXX.h.


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D125402#3517865 , @nlee wrote:

> 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:

I don't think that will be quite correct -- IIRC, that would still return true 
if the move constructor was deleted. `hasSimpleMoveConstructor()` and 
`hasSimpleMoveAssignment()` might be a better approach.

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

Those are good; I'd also run it over LLVM itself. Boost would also be a good 
thing to test against.

> 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 :)

No problem, Phabricator takes some getting used to and the Clang code base is 
pretty large. :-)




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

nlee wrote:
> 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"`
Yeah, we calculate move/copy constructor/assignment (triviality, etc) while 
building up the type for the class as we add declarations to it. So if we've 
not seen the definition yet, we won't be able to check that property.

That said, it might make sense to only trigger this diagnostic when *defining* 
the function because the parameter and return types must be complete by that 
point.



Comment at: clang/test/SemaCXX/warn-return-by-const-value.cpp:33
+}
\ No newline at end of file


You should add the newline to the end of the file.


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.

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



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

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: aaron.ballman, clang-language-wg.
aaron.ballman added a comment.

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.


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

[Chris Lattner via Phabricator via cfe-commits]

lattner edited reviewers, added: rsmith; removed: lattner.
lattner added a comment.

I'm not a competent reviewer for this, Richard can you recommend someone?


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 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