[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-11-16 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 abandoned this revision.
xbolva00 added a comment.

https://reviews.llvm.org/D70342


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-11-12 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added a comment.

Yes, i will prepare a new patch and add him as a reviewer.


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-11-12 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D68185#1742809 , @xbolva00 wrote:

> Ah, Clang has it under -Wdeprecated. 
> https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated.
>
> But combo -Wall -Wextra does not enable this warning, sadly.
>
> This should be extracted to -Wdeprecated-copy and put into -Wextra. Seems 
> fine for you?


Sounds plausible to me - though I'd expect @rsmith should probably have a final 
say before that's submitted.


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-11-12 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added a comment.

Ah, Clang has it under -Wdeprecated. 
https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated.

But combo -Wall -Wextra does not enable this warning, sadly.

This should be extracted to -Wdeprecated-copy and put into -Wextra. Seems fine 
for you?


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-11-12 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

Yeah, I'm OK with warning on the deprecated cases (especially for symmetry with 
GCC's warning) - not sure why that didn't occur to me/why I didn't mention it 
in my previous comment...

Deprecation wording is, in C++17, 15.8.1 [class.copy.ctor] p6, and 15.8.2 
[class.copy.assign] p2.


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-11-12 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added a comment.

Should we reconsider this as a clang warning? I found that the newest GCC can 
diagnose it.

GCC 9+:

-Wdeprecated-copy, implied by -Wextra, warns about the C++11 deprecation of 
implicitly declared copy constructor and assignment operator if one of them is 
user-provided. -Wdeprecated-copy-dtor also warns if the destructor is 
user-provided, as specified in C++11.

It woule be nice to keep compatibility. -Wextra seems reasonable for me too.


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-09-30 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

+1 for clang-tidy. GCC's closest to this is -Weffc++ which limits the rule of 3 
warning to only "classes that have dynamic memory allocation" (though I'm not 
sure exactly what conditions it uses to decide that - it doesn't warn on 
anything in the test cases provided in this patch, it does warn if you add a 
dtor to Clz that deletes buf:
rule3.cpp:1:7: warning: ‘class Clz’ has pointer data members [-Weffc++]
 class Clz // expected-warning {{class implements custom copy assignment 
operator but missing custom copy constructor}}

  ^~~

rule3.cpp:1:7: warning:   but does not override ‘Clz(const Clz&)’ [-Weffc++]


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-09-29 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 abandoned this revision.
xbolva00 added a comment.

Yeah, if we want to check rule of 3, clang-tidy is better choice.


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-09-29 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

Since https://en.cppreference.com/w/cpp/language/rule_of_three says "almost 
always" (note "almost") I'd say this probably belongs more in clang-tidy 
territory – I'm guessing false positive rate is very high for this. Can you 
collect some true / false positive statistics on some large open-source project 
(chromium, firefox, open office, …)?

Since C++11 it's rule of 0/3/5, so this should probably handle the move 
ctor/assign op too.


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-09-28 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added a comment.

>> if I provide a destructor but implicitly default my copy operations, isn't 
>> that just as bad, Rule-of-Three-wise?

Yeah, I am wondering about this too. whether we should check the whole 
Rule-of-Three. Suggestions from other reviewers?


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-09-28 Thread Arthur O'Dwyer via Phabricator via cfe-commits 
Quuxplusone added a comment.

Please add test cases showing the intended behavior for

- when the copy constructor is explicitly defaulted but the copy assignment 
operator is {implicitly defaulted, implicitly deleted, user-provided}
- when the copy assignment operator is explicitly defaulted but the copy 
constructor is {implicitly defaulted, implicitly deleted, user-provided}
- when the {copy constructor, copy assignment operator} is user-provided but 
the other is implicitly deleted

An example of an implicitly deleted copy assignment operator would be

  struct A {
  int& x;
  A(const A& rhs) : x(rhs.x) {}
  };

Also, how does the presence of a user-provided destructor interact with this 
diagnostic? If I provide a destructor but implicitly default my copy 
operations, isn't that just as bad, Rule-of-Three-wise?


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

https://reviews.llvm.org/D68185



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

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-09-28 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 created this revision.
xbolva00 added a reviewer: rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
xbolva00 updated this revision to Diff 222301.

Clang version of https://www.viva64.com/en/w/v690/
Also requested by Chromium developers:
https://bugs.chromium.org/p/chromium/issues/detail?id=979077=component%3ATools%3ELLVM=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified


https://reviews.llvm.org/D68185

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclCXX.cpp
  test/SemaCXX/warn-custom-copy.cpp


Index: test/SemaCXX/warn-custom-copy.cpp
===
--- test/SemaCXX/warn-custom-copy.cpp
+++ test/SemaCXX/warn-custom-copy.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wcustom-copy %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class Clz // expected-warning {{class implements custom copy assignment 
operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz() : buf(nullptr) {}
+  Clz =(const Clz ) {
+buf = r.buf;
+return *this;
+  }
+};
+
+class Clz2 // expected-warning {{class implements custom copy assignment 
operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz2() : buf(nullptr) {}
+  Clz2(const Clz2 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+};
+
+class Clz3 {
+  char *buf;
+
+public:
+  Clz3() : buf(nullptr) {}
+  Clz3(const Clz3 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+  Clz3 =(const Clz3 ) {
+buf = r.buf;
+return *this;
+  }
+};
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -6279,6 +6279,14 @@
 }
   }
 
+  // Warn if the class implements custom copy constructor but missing
+  // custom copy assignment operator and vice versa.
+  if (Record->hasUserDeclaredCopyAssignment() !=
+  Record->hasUserDeclaredCopyConstructor())
+Diag(Record->getLocation(), diag::warn_missing_custom_copy)
+<< Record->hasUserDeclaredCopyConstructor()
+<< !Record->hasUserDeclaredCopyConstructor();
+
   // Warn if the class has virtual methods but non-virtual public destructor.
   if (Record->isPolymorphic() && !Record->isDependentType()) {
 CXXDestructorDecl *dtor = Record->getDestructor();
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2245,6 +2245,11 @@
 def note_final_dtor_non_final_class_silence : Note<
   "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
+def warn_missing_custom_copy : Warning<
+  "class implements custom %select{copy assignment operator|copy constructor}0 
"
+  "but missing custom %select{copy assignment operator|copy constructor}1">,
+  InGroup>;
+
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
 


Index: test/SemaCXX/warn-custom-copy.cpp
===
--- test/SemaCXX/warn-custom-copy.cpp
+++ test/SemaCXX/warn-custom-copy.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wcustom-copy %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class Clz // expected-warning {{class implements custom copy assignment operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz() : buf(nullptr) {}
+  Clz =(const Clz ) {
+buf = r.buf;
+return *this;
+  }
+};
+
+class Clz2 // expected-warning {{class implements custom copy assignment operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz2() : buf(nullptr) {}
+  Clz2(const Clz2 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+};
+
+class Clz3 {
+  char *buf;
+
+public:
+  Clz3() : buf(nullptr) {}
+  Clz3(const Clz3 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+  Clz3 =(const Clz3 ) {
+buf = r.buf;
+return *this;
+  }
+};
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -6279,6 +6279,14 @@
 }
   }
 
+  // Warn if the class implements custom copy constructor but missing
+  // custom copy assignment operator and vice versa.
+  if (Record->hasUserDeclaredCopyAssignment() !=
+  Record->hasUserDeclaredCopyConstructor())
+Diag(Record->getLocation(), diag::warn_missing_custom_copy)
+<< Record->hasUserDeclaredCopyConstructor()
+<< !Record->hasUserDeclaredCopyConstructor();
+
   // Warn if the class has virtual methods but non-virtual public destructor.
   if (Record->isPolymorphic() && !Record->isDependentType()) {
 CXXDestructorDecl *dtor = Record->getDestructor();
Index: include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D68185: [Diagnostics] Warn when class implements a copy constructor/copy assignment operator, but missing the copy assignment operator/copy constructor

2019-09-28 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 updated this revision to Diff 222301.

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

https://reviews.llvm.org/D68185

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclCXX.cpp
  test/SemaCXX/warn-custom-copy.cpp


Index: test/SemaCXX/warn-custom-copy.cpp
===
--- test/SemaCXX/warn-custom-copy.cpp
+++ test/SemaCXX/warn-custom-copy.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wcustom-copy %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class Clz // expected-warning {{class implements custom copy assignment 
operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz() : buf(nullptr) {}
+  Clz =(const Clz ) {
+buf = r.buf;
+return *this;
+  }
+};
+
+class Clz2 // expected-warning {{class implements custom copy assignment 
operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz2() : buf(nullptr) {}
+  Clz2(const Clz2 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+};
+
+class Clz3 {
+  char *buf;
+
+public:
+  Clz3() : buf(nullptr) {}
+  Clz3(const Clz3 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+  Clz3 =(const Clz3 ) {
+buf = r.buf;
+return *this;
+  }
+};
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -6279,6 +6279,14 @@
 }
   }
 
+  // Warn if the class implements custom copy constructor but missing
+  // custom copy assignment operator and vice versa.
+  if (Record->hasUserDeclaredCopyAssignment() !=
+  Record->hasUserDeclaredCopyConstructor())
+Diag(Record->getLocation(), diag::warn_missing_custom_copy)
+<< Record->hasUserDeclaredCopyConstructor()
+<< !Record->hasUserDeclaredCopyConstructor();
+
   // Warn if the class has virtual methods but non-virtual public destructor.
   if (Record->isPolymorphic() && !Record->isDependentType()) {
 CXXDestructorDecl *dtor = Record->getDestructor();
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2245,6 +2245,11 @@
 def note_final_dtor_non_final_class_silence : Note<
   "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
+def warn_missing_custom_copy : Warning<
+  "class implements custom %select{copy assignment operator|copy constructor}0 
"
+  "but missing custom %select{copy assignment operator|copy constructor}1">,
+  InGroup>;
+
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
 


Index: test/SemaCXX/warn-custom-copy.cpp
===
--- test/SemaCXX/warn-custom-copy.cpp
+++ test/SemaCXX/warn-custom-copy.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wcustom-copy %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class Clz // expected-warning {{class implements custom copy assignment operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz() : buf(nullptr) {}
+  Clz =(const Clz ) {
+buf = r.buf;
+return *this;
+  }
+};
+
+class Clz2 // expected-warning {{class implements custom copy assignment operator but missing custom copy constructor}}
+
+{
+  char *buf;
+
+public:
+  Clz2() : buf(nullptr) {}
+  Clz2(const Clz2 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+};
+
+class Clz3 {
+  char *buf;
+
+public:
+  Clz3() : buf(nullptr) {}
+  Clz3(const Clz3 ) : buf(nullptr) {
+(void)buf;
+(void)r;
+  }
+  Clz3 =(const Clz3 ) {
+buf = r.buf;
+return *this;
+  }
+};
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -6279,6 +6279,14 @@
 }
   }
 
+  // Warn if the class implements custom copy constructor but missing
+  // custom copy assignment operator and vice versa.
+  if (Record->hasUserDeclaredCopyAssignment() !=
+  Record->hasUserDeclaredCopyConstructor())
+Diag(Record->getLocation(), diag::warn_missing_custom_copy)
+<< Record->hasUserDeclaredCopyConstructor()
+<< !Record->hasUserDeclaredCopyConstructor();
+
   // Warn if the class has virtual methods but non-virtual public destructor.
   if (Record->isPolymorphic() && !Record->isDependentType()) {
 CXXDestructorDecl *dtor = Record->getDestructor();
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2245,6 +2245,11 @@
 def note_final_dtor_non_final_class_silence : Note<
   "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
+def warn_missing_custom_copy : Warning<
+  "class implements custom