From 09464e63d842e1e0e6be300b6bb59bc9c1e7ecdf Mon Sep 17 00:00:00 2001 From: Alexander Kornienko Date: Mon, 4 Apr 2016 14:31:36 +0000 Subject: [PATCH] [clang-tidy] fix a couple of modernize-use-override bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix for __declspec attributes and const=0 without space This patch is to address 2 problems I found with Clang-tidy:modernize-use-override. 1: missing spaces on pure function decls. Orig: void pure() const=0 Problem: void pure() constoverride =0 Fixed: void pure() const override =0 2: This is ms-extension specific, but possibly applies to other attribute types. The override is placed before the attribute which doesn’t work well with declspec as this attribute can be inherited or placed before the method identifier. Orig: class __declspec(dllexport) X : public Y { void p(); }; Problem: class override __declspec(dllexport) class X : public Y { void p(); }; Fixed: class __declspec(dllexport) class X : public Y { void p() override; }; Patch by Robert Bolter! Differential Revision: http://reviews.llvm.org/D18396 llvm-svn: 265298 --- .../clang-tidy/modernize/UseOverrideCheck.cpp | 20 +++++++++------ clang-tools-extra/docs/ReleaseNotes.rst | 7 ++++-- .../clang-tidy/modernize-use-override-ms.cpp | 25 +++++++++++++++++++ .../clang-tidy/modernize-use-override.cpp | 7 +++++- 4 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/modernize-use-override-ms.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp index a335748301cd..81fa73f95c53 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp @@ -95,9 +95,9 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { : "'override' is"; StringRef Correct = HasFinal ? "'final'" : "'override'"; - Message = - (llvm::Twine(Redundant) + - " redundant since the function is already declared " + Correct).str(); + Message = (llvm::Twine(Redundant) + + " redundant since the function is already declared " + Correct) + .str(); } DiagnosticBuilder Diag = diag(Method->getLocation(), Message); @@ -118,9 +118,11 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { if (!HasFinal && !HasOverride) { SourceLocation InsertLoc; StringRef ReplacementText = "override "; + SourceLocation MethodLoc = Method->getLocation(); for (Token T : Tokens) { - if (T.is(tok::kw___attribute)) { + if (T.is(tok::kw___attribute) && + !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) { InsertLoc = T.getLocation(); break; } @@ -128,11 +130,12 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { if (Method->hasAttrs()) { for (const clang::Attr *A : Method->getAttrs()) { - if (!A->isImplicit()) { + if (!A->isImplicit() && !A->isInherited()) { SourceLocation Loc = Sources.getExpansionLoc(A->getRange().getBegin()); - if (!InsertLoc.isValid() || - Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) + if ((!InsertLoc.isValid() || + Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) && + !Sources.isBeforeInTranslationUnit(Loc, MethodLoc)) InsertLoc = Loc; } } @@ -163,6 +166,9 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) { Tokens.back().is(tok::kw_delete)) && GetText(Tokens[Tokens.size() - 2], Sources) == "=") { InsertLoc = Tokens[Tokens.size() - 2].getLocation(); + // Check if we need to insert a space. + if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0) + ReplacementText = " override "; } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") { InsertLoc = Tokens.back().getLocation(); } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 7428c9361705..7b65a023cb6d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -155,9 +155,12 @@ identified. The improvements since the 3.8 release include: Fixed bugs: - Crash when running on compile database with relative source files paths. +- Crash when running on compile database with relative source files paths. - Crash when running with the `-fdelayed-template-parsing` flag. +- Crash when running with the `-fdelayed-template-parsing` flag. + +- The ``modernize-use-override`` check: incorrect fix-its placement around + ``__declspec`` and other attributes. Clang-tidy changes from 3.7 to 3.8 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/modernize-use-override-ms.cpp b/clang-tools-extra/test/clang-tidy/modernize-use-override-ms.cpp new file mode 100644 index 000000000000..0cee91708375 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/modernize-use-override-ms.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy %s modernize-use-override %t -- -- -fms-extensions -std=c++11 + +// This test is designed to test ms-extension __declspec(dllexport) attributes. +#define EXPORT __declspec(dllexport) + +class Base { + virtual EXPORT void a(); +}; + +class EXPORT InheritedBase { + virtual void a(); +}; + +class Derived : public Base { + virtual EXPORT void a(); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override] + // CHECK-FIXES: {{^}} EXPORT void a() override; +}; + +class EXPORT InheritedDerived : public InheritedBase { + virtual void a(); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override] + // CHECK-FIXES: {{^}} void a() override; +}; + diff --git a/clang-tools-extra/test/clang-tidy/modernize-use-override.cpp b/clang-tools-extra/test/clang-tidy/modernize-use-override.cpp index e4be332e9ed8..1e39e37d210a 100644 --- a/clang-tools-extra/test/clang-tidy/modernize-use-override.cpp +++ b/clang-tools-extra/test/clang-tidy/modernize-use-override.cpp @@ -21,6 +21,7 @@ struct Base { virtual void d2(); virtual void e() = 0; virtual void f() = 0; + virtual void f2() const = 0; virtual void g() = 0; virtual void j() const; @@ -74,7 +75,11 @@ public: virtual void f()=0; // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using - // CHECK-FIXES: {{^}} void f()override =0; + // CHECK-FIXES: {{^}} void f() override =0; + + virtual void f2() const=0; + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using + // CHECK-FIXES: {{^}} void f2() const override =0; virtual void g() ABSTRACT; // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using