[clang-tidy] Changes to modernize-use-default check

Summary:
Warn about special member functions that only contain a comment.
Report the location of the special member function, unless it is
defined in a macro.  Reporting the location of the body in a macro is
more helpful as it causes the macro expansion location to be reported too.

Fixes PR30920.

Reviewers: alexfh, aaron.ballman

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D26741

llvm-svn: 287215

3 files changed, 71 insertions, 24 deletions

diff --git a/clang-tools-extra/clang-tidy/modernize/UseDefaultCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDefaultCheck.cpp
index 6092ae65e54..09595f48196 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseDefaultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseDefaultCheck.cpp
@@ -249,11 +249,14 @@ void UseDefaultCheck::check(const MatchFinder::MatchResult &Result) {
   if (!Body)
     return;
 
-  // If there are comments inside the body, don't do the change.
-  if (!SpecialFunctionDecl->isCopyAssignmentOperator() &&
-      !bodyEmpty(Result.Context, Body))
+  // If there is code inside the body, don't warn.
+  if (!SpecialFunctionDecl->isCopyAssignmentOperator() && !Body->body_empty())
     return;
 
+  // If there are comments inside the body, don't do the change.
+  bool ApplyFix = SpecialFunctionDecl->isCopyAssignmentOperator() ||
+                  bodyEmpty(Result.Context, Body);
+
   std::vector<FixItHint> RemoveInitializers;
 
   if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(SpecialFunctionDecl)) {
@@ -277,10 +280,18 @@ void UseDefaultCheck::check(const MatchFinder::MatchResult &Result) {
     SpecialFunctionName = "copy-assignment operator";
   }
 
-  diag(SpecialFunctionDecl->getLocStart(),
-       "use '= default' to define a trivial " + SpecialFunctionName)
-      << FixItHint::CreateReplacement(Body->getSourceRange(), "= default;")
-      << RemoveInitializers;
+  // The location of the body is more useful inside a macro as spelling and
+  // expansion locations are reported.
+  SourceLocation Location = SpecialFunctionDecl->getLocation();
+  if (Location.isMacroID())
+    Location = Body->getLocStart();
+
+  auto Diag = diag(Location, "use '= default' to define a trivial " +
+                                 SpecialFunctionName);
+
+  if (ApplyFix)
+    Diag << FixItHint::CreateReplacement(Body->getSourceRange(), "= default;")
+         << RemoveInitializers;
 }
 
 } // namespace modernize
diff --git a/clang-tools-extra/test/clang-tidy/modernize-use-default-copy.cpp b/clang-tools-extra/test/clang-tidy/modernize-use-default-copy.cpp
index 59317ce192e..0a349fbba00 100644
--- a/clang-tools-extra/test/clang-tidy/modernize-use-default-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/modernize-use-default-copy.cpp
@@ -7,13 +7,13 @@ struct OL {
   int Field;
 };
 OL::OL(const OL &Other) : Field(Other.Field) {}
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial copy constructor [modernize-use-default]
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use '= default' to define a trivial copy constructor [modernize-use-default]
 // CHECK-FIXES: OL::OL(const OL &Other)  = default;
 OL &OL::operator=(const OL &Other) {
   Field = Other.Field;
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-4]]:1: warning: use '= default' to define a trivial copy-assignment operator [modernize-use-default]
+// CHECK-MESSAGES: :[[@LINE-4]]:9: warning: use '= default' to define a trivial copy-assignment operator [modernize-use-default]
 // CHECK-FIXES: OL &OL::operator=(const OL &Other) = default;
 
 // Inline.
@@ -25,7 +25,7 @@ struct IL {
     Field = Other.Field;
     return *this;
   }
-  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use '= default'
+  // CHECK-MESSAGES: :[[@LINE-4]]:7: warning: use '= default'
   // CHECK-FIXES: IL &operator=(const IL &Other) = default;
   int Field;
 };
@@ -110,7 +110,7 @@ struct Empty {
   Empty &operator=(const Empty &);
 };
 Empty &Empty::operator=(const Empty &Other) { return *this; }
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use '= default'
 // CHECK-FIXES: Empty &Empty::operator=(const Empty &Other) = default;
 
 // Bit fields.
@@ -137,7 +137,7 @@ BF &BF::operator=(const BF &Other) {
   Field4 = Other.Field4;
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-7]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-7]]:9: warning: use '= default'
 // CHECK-FIXES: BF &BF::operator=(const BF &Other) = default;
 
 // Base classes.
@@ -153,7 +153,7 @@ BC &BC::operator=(const BC &Other) {
   BF::operator=(Other);
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-6]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-6]]:9: warning: use '= default'
 // CHECK-FIXES: BC &BC::operator=(const BC &Other) = default;
 
 // Base classes with member.
@@ -170,7 +170,7 @@ BCWM &BCWM::operator=(const BCWM &Other) {
   Bf = Other.Bf;
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-6]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-6]]:13: warning: use '= default'
 // CHECK-FIXES: BCWM &BCWM::operator=(const BCWM &Other) = default;
 
 // Missing base class.
@@ -213,7 +213,7 @@ VBC &VBC::operator=(const VBC &Other) {
   VB::operator=(Other);
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-6]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-6]]:11: warning: use '= default'
 // CHECK-FIXES: VBC &VBC::operator=(const VBC &Other) = default;
 
 // Indirect base.
@@ -291,7 +291,7 @@ DA &DA::operator=(const DA &Other) {
   Field2 = Other.Field2;
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-5]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-5]]:9: warning: use '= default'
 // CHECK-FIXES: DA &DA::operator=(const DA &Other) = default;
 
 struct DA2 {
@@ -324,6 +324,7 @@ SIB &SIB::operator=(const SIB &Other) {
 struct CIB {
   CIB(const CIB &Other) : Field(Other.Field) { /* Don't erase this */
   }
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use '= default'
   CIB &operator=(const CIB &);
   int Field;
 };
@@ -332,7 +333,7 @@ CIB &CIB::operator=(const CIB &Other) {
   // FIXME: don't erase this comment.
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-5]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-5]]:11: warning: use '= default'
 // CHECK-FIXES: CIB &CIB::operator=(const CIB &Other) = default;
 
 // Take non-const reference as argument.
@@ -348,7 +349,7 @@ NCRef &NCRef::operator=(NCRef &Other) {
   Field2 = Other.Field2;
   return *this;
 }
-// CHECK-MESSAGES: :[[@LINE-5]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-5]]:15: warning: use '= default'
 // CHECK-FIXES: NCRef &NCRef::operator=(NCRef &Other) = default;
 
 // Already defaulted.
@@ -471,3 +472,26 @@ struct NEF {
   NEF &operator=(const NEF &Other) noexcept(false);
 };
 //NEF &NEF::operator=(const NEF &Other) noexcept(false) { return *this; }
+
+#define STRUCT_WITH_COPY_CONSTRUCT(_base, _type) \
+  struct _type {                                 \
+    _type(const _type &v) : value(v.value) {}    \
+    _base value;                                 \
+  };
+
+STRUCT_WITH_COPY_CONSTRUCT(unsigned char, Hex8CopyConstruct)
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial copy constructor
+// CHECK-MESSAGES: :[[@LINE-6]]:44: note:
+
+#define STRUCT_WITH_COPY_ASSIGN(_base, _type) \
+  struct _type {                              \
+    _type &operator=(const _type &rhs) {      \
+      value = rhs.value;                      \
+      return *this;                           \
+    }                                         \
+    _base value;                              \
+  };
+
+STRUCT_WITH_COPY_ASSIGN(unsigned char, Hex8CopyAssign)
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial copy-assignment operator
+// CHECK-MESSAGES: :[[@LINE-9]]:40: note:
diff --git a/clang-tools-extra/test/clang-tidy/modernize-use-default.cpp b/clang-tools-extra/test/clang-tidy/modernize-use-default.cpp
index 6f6f01fa1c8..14ca1bc980f 100644
--- a/clang-tools-extra/test/clang-tidy/modernize-use-default.cpp
+++ b/clang-tools-extra/test/clang-tidy/modernize-use-default.cpp
@@ -8,10 +8,10 @@ public:
 };
 
 OL::OL() {}
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial default constructor [modernize-use-default]
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use '= default' to define a trivial default constructor [modernize-use-default]
 // CHECK-FIXES: OL::OL() = default;
 OL::~OL() {}
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial destructor [modernize-use-default]
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use '= default' to define a trivial destructor [modernize-use-default]
 // CHECK-FIXES: OL::~OL() = default;
 
 // Inline definitions.
@@ -92,10 +92,10 @@ public:
 class KW {
 public:
   explicit KW() {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use '= default'
   // CHECK-FIXES: explicit KW() = default;
   virtual ~KW() {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use '= default'
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use '= default'
   // CHECK-FIXES: virtual ~KW() = default;
 };
 
@@ -134,11 +134,11 @@ public:
 
 template <class T>
 TempODef<T>::TempODef() {}
-// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use '= default'
 // CHECK-FIXES: TempODef<T>::TempODef() = default;
 template <class T>
 TempODef<T>::~TempODef() {}
-// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: use '= default'
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use '= default'
 // CHECK-FIXES: TempODef<T>::~TempODef() = default;
 
 template class TempODef<int>;
@@ -178,9 +178,11 @@ struct Comments {
   Comments() {
     // Don't erase comments inside the body.
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use '= default'
   ~Comments() {
     // Don't erase comments inside the body.
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use '= default'
 };
 
 // Try-catch.
@@ -195,3 +197,13 @@ struct OTC {
 };
 OTC::OTC() try {} catch(...) {}
 OTC::~OTC() try {} catch(...) {}
+
+#define STRUCT_WITH_DEFAULT(_base, _type) \
+  struct _type {                          \
+    _type() {}                            \
+    _base value;                          \
+  };
+
+STRUCT_WITH_DEFAULT(unsigned char, Hex8Default)
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use '= default' to define a trivial default constructor
+// CHECK-MESSAGES: :[[@LINE-6]]:13: note: