[PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Robert Bolter via cfe-commits]

Rob created this revision.
Rob added reviewers: alexfh, aaron.ballman.
Rob added a subscriber: cfe-commits.

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;
  };


http://reviews.llvm.org/D18396

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  test/clang-tidy/modernize-use-override-ms.cpp
  test/clang-tidy/modernize-use-override.cpp


Index: test/clang-tidy/modernize-use-override.cpp
===================================================================
--- test/clang-tidy/modernize-use-override.cpp
+++ test/clang-tidy/modernize-use-override.cpp
@@ -21,6 +21,7 @@
   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 @@
 
   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
Index: test/clang-tidy/modernize-use-override-ms.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/modernize-use-override-ms.cpp
@@ -0,0 +1,45 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t
+
+// This attribute type is inherited and precedes the declaration
+#define EXPORT __declspec(dllexport)
+
+class EXPORT ExpBaseMacro {
+  virtual void a();
+};
+
+class __declspec(dllexport) ExpBase {
+  virtual void a();
+};
+
+class BaseMacro {
+  virtual EXPORT void a();
+};
+
+class Base {
+  virtual __declspec(dllexport) void a();
+};
+
+
+class EXPORT ExpDerivedMacro : public ExpBaseMacro {
+  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;
+};
+
+class __declspec(dllexport) ExpDerived : public ExpBase {
+  virtual void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void a() override;
+};
+
+class DerivedMacro : public BaseMacro {
+  virtual EXPORT void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: prefer using
+  // CHECK-FIXES: {{^}}  EXPORT void a() override;
+};
+
+class Derived : public Base {
+  virtual __declspec(dllexport) void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: prefer using
+  // CHECK-FIXES: {{^}}  __declspec(dllexport) void a() override;
+};
Index: clang-tidy/modernize/UseOverrideCheck.cpp
===================================================================
--- clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tidy/modernize/UseOverrideCheck.cpp
@@ -118,25 +118,28 @@
   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;
       }
     }
 
     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;
+          }
         }
       }
-    }
 
     if (InsertLoc.isInvalid() && Method->doesThisDeclarationHaveABody() &&
         Method->getBody() && !Method->isDefaulted()) {
@@ -163,6 +166,9 @@
                                 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();
       }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits