[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb70e6e968192: [clang-tidy] Warn only for copyable/movable 
classes in cppcoreguidelines-avoid… (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = 

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL accepted this revision.
PiotrZSL added a comment.

Looks fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

carlosgalvezp wrote:
> PiotrZSL wrote:
> > carlosgalvezp wrote:
> > > PiotrZSL wrote:
> > > > Check first type, should be cheaper and consider mering those two. 
> > > Thanks for the tip! I'm not familiar with having multiple binds in the 
> > > same `addMatcher` call. Do I still need to keep the `bind("ref")` at the 
> > > end?
> > No, bind at the end need to be removed (I forgot about that).
> Somehow I didn't manage to get it to work with your proposed change and 
> removing `bind`, I feel I'm walking on thin ice here :) 
Doing it from scratch helped, probably some parenthesis that was off!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 541963.
carlosgalvezp added a comment.

Merge matchers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Using `hasSimpleCopyConstructor` and so on greatly simplifies the logic, great! 
Let me know if you are happy with it or I should go ahead and merge.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

PiotrZSL wrote:
> carlosgalvezp wrote:
> > PiotrZSL wrote:
> > > Check first type, should be cheaper and consider mering those two. 
> > Thanks for the tip! I'm not familiar with having multiple binds in the same 
> > `addMatcher` call. Do I still need to keep the `bind("ref")` at the end?
> No, bind at the end need to be removed (I forgot about that).
Somehow I didn't manage to get it to work with your proposed change and 
removing `bind`, I feel I'm walking on thin ice here :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 541961.
carlosgalvezp marked 3 inline comments as done.
carlosgalvezp added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: 

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

carlosgalvezp wrote:
> PiotrZSL wrote:
> > Check first type, should be cheaper and consider mering those two. 
> Thanks for the tip! I'm not familiar with having multiple binds in the same 
> `addMatcher` call. Do I still need to keep the `bind("ref")` at the end?
No, bind at the end need to be removed (I forgot about that).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D155625#4512123 , @PiotrZSL wrote:

> LGTM, but I'm not sure if isCopyableOrMovable will always work correctly, for 
> a user defined special members it looks ok, but for some implicit ones I 
> worry it may not always work. Probably things like 
> "(hasSimpleCopyAssigment()) || (hasUserDeclaredCopyAssigment() && check here 
> if its not deleted)" would be needed.
> Thing is that CXXRecordDecl got most info (in confused way), there are things 
> like DefaultedMoveConstructorIsDeleted that could be used to verify somehow 
> base class.

Yeah I spent a lot of time trying to figure this out, somehow I expected this 
logic to already exist somewhere in Sema! Thanks for the pointers, I will 
experiment with those functions as well.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

PiotrZSL wrote:
> Check first type, should be cheaper and consider mering those two. 
Thanks for the tip! I'm not familiar with having multiple binds in the same 
`addMatcher` call. Do I still need to keep the `bind("ref")` at the end?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL accepted this revision.
PiotrZSL added a comment.
This revision is now accepted and ready to land.

LGTM, but I'm not sure if isCopyableOrMovable will always work correctly, for a 
user defined special members it looks ok, but for some implicit ones I worry it 
may not always work. Probably things like "(hasSimpleCopyAssigment()) || 
(hasUserDeclaredCopyAssigment() && check here if its not deleted)" would be 
needed.
Thing is that CXXRecordDecl got most info (in confused way), there are things 
like DefaultedMoveConstructorIsDeleted that could be used to verify somehow 
base class.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp:91-102
+  Finder->addMatcher(
+  fieldDecl(unless(isMemberOfLambda()),
+hasDeclContext(cxxRecordDecl(isCopyableOrMovable())),
+hasType(hasCanonicalType(referenceType(
+  .bind("ref"),
+  this);
+  Finder->addMatcher(

Check first type, should be cheaper and consider mering those two. 



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst:6
 
-This check warns when structs or classes have const-qualified or reference
-(lvalue or rvalue) data members. Having such members is rarely useful, and
-makes the class only copy-constructible but not copy-assignable.
+This check warns when structs or classes that are copyable or movable have
+const-qualified or reference (lvalue or rvalue) data members. Having such




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

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

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 541684.
carlosgalvezp added a comment.

Split getInfo function into 2 functions, remove
structured bindings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: 

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 541626.
carlosgalvezp added a comment.

Remove extra newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,130 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 541624.
carlosgalvezp added a comment.

Fix typo in release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,131 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 541623.
carlosgalvezp added a comment.

Remove confusing comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,131 @@
 auto c5 = x5;
   };
 }
+
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+  NonMovableCopiesNotDeclared(NonMovableCopiesNotDeclared&&) = delete;
+  NonMovableCopiesNotDeclared& operator=(NonMovableCopiesNotDeclared&&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst

[PATCH] D155625: [clang-tidy] Warn only for copyable/movable classes in cppcoreguidelines-avoid-const-or-ref-members

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, shchenz, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Since that's what the guidelines require.

Fixes #63733


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155625

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
@@ -205,3 +205,132 @@
 auto c5 = x5;
   };
 }
+
+// Classes without any copy or move operation are exempted from the rule
+struct NonCopyableWithRef
+{
+  NonCopyableWithRef(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef& operator=(NonCopyableWithRef const&) = delete;
+  NonCopyableWithRef(NonCopyableWithRef&&) = default;
+  NonCopyableWithRef& operator=(NonCopyableWithRef&&) = default;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonMovableWithRef
+{
+  NonMovableWithRef(NonMovableWithRef const&) = default;
+  NonMovableWithRef& operator=(NonMovableWithRef const&) = default;
+  NonMovableWithRef(NonMovableWithRef&&) = delete;
+  NonMovableWithRef& operator=(NonMovableWithRef&&) = delete;
+
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct NonCopyableNonMovableWithRef
+{
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef(NonCopyableNonMovableWithRef&&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef const&) = delete;
+  NonCopyableNonMovableWithRef& operator=(NonCopyableNonMovableWithRef&&) = delete;
+
+  int& x; // OK, non copyable nor movable
+};
+
+struct NonCopyable
+{
+  NonCopyable(NonCopyable const&) = delete;
+  NonCopyable& operator=(NonCopyable const&) = delete;
+  NonCopyable(NonCopyable&&) = default;
+  NonCopyable& operator=(NonCopyable&&) = default;
+};
+
+struct NonMovable
+{
+  NonMovable(NonMovable const&) = default;
+  NonMovable& operator=(NonMovable const&) = default;
+  NonMovable(NonMovable&&) = delete;
+  NonMovable& operator=(NonMovable&&) = delete;
+};
+
+struct NonCopyableNonMovable
+{
+  NonCopyableNonMovable(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable const&) = delete;
+  NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
+};
+
+// Test inheritance
+struct InheritFromNonCopyable : NonCopyable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonMovable : NonMovable
+{
+  int& x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'x' of type 'int &' is a reference
+};
+
+struct InheritFromNonCopyableNonMovable : NonCopyableNonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+struct InheritBothFromNonCopyableAndNonMovable : NonCopyable, NonMovable
+{
+  int& x;  // OK, non copyable nor movable
+};
+
+// Test composition
+struct ContainsNonCopyable
+{
+  NonCopyable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonMovable
+{
+  NonMovable x;
+  int& y;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'y' of type 'int &' is a reference
+};
+
+struct ContainsNonCopyableNonMovable
+{
+  NonCopyableNonMovable x;
+  int& y;  // OK, non copyable nor movable
+};
+
+
+struct ContainsBothNonCopyableAndNonMovable
+{
+  NonCopyable x;
+  NonMovable y;
+  int& z;  // OK, non copyable nor movable
+};
+
+// If copies are deleted and moves are not declared, moves are not implicitly declared,
+// so the class is also not movable and we should not warn
+struct NonCopyableMovesNotDeclared
+{
+  NonCopyableMovesNotDeclared(NonCopyableMovesNotDeclared const&) = delete;
+  NonCopyableMovesNotDeclared& operator=(NonCopyableMovesNotDeclared const&) = delete;
+
+  int& x;  // OK, non copyable nor movable
+};
+
+// If moves are deleted but copies are not declared, copies are implicitly deleted,
+// so the class is also not copyable and we should not warn
+struct NonMovableCopiesNotDeclared
+{
+