From 5be5092c49191881b2baa8cbce6418a914f6b8e9 Mon Sep 17 00:00:00 2001 From: Malcolm Parsons Date: Thu, 17 Nov 2016 09:14:04 +0000 Subject: [PATCH] [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 --- .../clang-tidy/modernize/UseDefaultCheck.cpp | 25 ++++++++---- .../test/clang-tidy/modernize-use-default-copy.cpp | 46 ++++++++++++++++------ .../test/clang-tidy/modernize-use-default.cpp | 24 ++++++++--- 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 6092ae6..09595f4 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 RemoveInitializers; if (const auto *Ctor = dyn_cast(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 59317ce..0a349fb 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 6f6f01f..14ca1bc 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 TempODef::TempODef() {} -// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: use '= default' +// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use '= default' // CHECK-FIXES: TempODef::TempODef() = default; template TempODef::~TempODef() {} -// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: use '= default' +// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use '= default' // CHECK-FIXES: TempODef::~TempODef() = default; template class TempODef; @@ -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: -- 2.7.4