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

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Mon Apr  4 09:31:36 2016
New Revision: 265298

URL: http://llvm.org/viewvc/llvm-project?rev=265298&view=rev
Log:
[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

Added:
    clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp
Modified:
    clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
    clang-tools-extra/trunk/docs/ReleaseNotes.rst
    clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp

Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp?rev=265298&r1=265297&r2=265298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp Mon Apr  4 09:31:36 2016
@@ -95,9 +95,9 @@ void UseOverrideCheck::check(const Match
                    : "'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 Match
   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 Match
 
     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 Match
                                 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();
       }

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=265298&r1=265297&r2=265298&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Mon Apr  4 09:31:36 2016
@@ -155,9 +155,12 @@ identified.  The improvements since the
 
 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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Added: clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp?rev=265298&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp Mon Apr  4 09:31:36 2016
@@ -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;
+};
+

Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp?rev=265298&r1=265297&r2=265298&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp Mon Apr  4 09:31:36 2016
@@ -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