[clang-tidy] fix a couple of modernize-use-override bugs

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

4 files changed, 49 insertions, 10 deletions

diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
index a335748301c..81fa73f95c5 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 7428c936170..7b65a023cb6 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 00000000000..0cee9170837
--- /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 e4be332e9ed..1e39e37d210 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