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

[Rob]

Summary

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

[alexfh]

Thank you for the patch!

Looks good in general. A few nits.

clang-tidy/modernize/UseOverrideCheck.cpp

125

nit: no need for the parentheses around the function call.

136–138

Please clang-format this change.

test/clang-tidy/modernize-use-override-ms.cpp

2

Does this test pass on linux? If no additional compiler arguments needed and it just works, maybe just merge this test code to the main test file?

[Rob]

Update to address review.
Clang-format applied and nit fixed.

[aaron.ballman]

test/clang-tidy/modernize-use-override-ms.cpp

1

I *think* this run line may require -fms-extensions in order to compile under non-MSVC-built versions of clang because of the __declspec.

[LegalizeAdulthood]

Please summarize the improvements in docs/ReleaseNotes.rst in the clang tidy section.

[Rob]

I tried

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp -- -- -fms-extensions

and

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp

on OSX and nethier work as expected.

So instead I have changed the test to use visibility attributes when _MSC_VER is not defined.

Also updated the release notes.

[alexfh]

In D18396#388287, @Rob wrote:

I tried

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp -- -- -fms-extensions

It also needs -std=c++11, which needs to be specified, when you specify any options manually.

and

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp

on OSX and nethier work as expected.

So instead I have changed the test to use visibility attributes when _MSC_VER is not defined.

... which means that the code is not tested on non-windows platforms, though it could be.

Also updated the release notes.

Thanks!

test/clang-tidy/modernize-use-override-ms.cpp

1

I've just tried this on Linux and it actually needs -fms-extensions (actually, you'll need to append -- -- -std=c++11 -fms-extensions).

[alexfh]

In D18396#388287, @Rob wrote:

I tried

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp -- -- -fms-extensions

It also needs -std=c++11, since you specify options manually and "opt out" of the defaults (which are currently -std=c++11).

and

check_clang_tidy.py modernize-use-override-ms.cpp modernize-use-override temp

on OSX and nethier work as expected.

So instead I have changed the test to use visibility attributes when _MSC_VER is not defined.

... which means that the code is not tested on non-windows platforms, though it could be.

Also updated the release notes.

Thanks!

docs/ReleaseNotes.rst

73 ↗

(On Diff #52227)

nit: Please enclose __declspec in double backquotes.

[Rob]

adding -std=c++11 and thought it worked as expected so uploaded diff4.

In fact it is just ignoring the attributes, so its not actually testing the problem.

I propose going back to revision 3 where one of the two tests did actually fail on osx, without my fix. The test where the attribute on the class decl that is inherited by the function decl is not reproducable using attribute(visibility)

warning: __declspec attribute 'dllexport'

is not supported [-Wignored-attributes]

[alexfh]

Looks good with one nit. Thank you for the patch!

Please tell, if you need someone to commit the patch for you.

docs/ReleaseNotes.rst

73 ↗

(On Diff #52539)

Please enclose inline code snippets (__declspec) in double backquotes.

[alexfh]

test/clang-tidy/modernize-use-override-ms.cpp

3

An old comment sneaked in somehow. Please ignore.

[Rob]

Adding back quotes in the docs

[Rob]

ok, I'm not sure if its worth rolling back the change to modernize-use-override-ms.cpp, see ammended comments above.

Apart from this it should be good to go.

I think I am going to need someone to submit the patch for me, I can't use svn due to 'error 400' so I've been using the git mirror.

test/clang-tidy/modernize-use-override-ms.cpp

2

That's a good question. I have been running it with the additional -fms-extensions argument, but on windows I find it works with or without that option. I havent tested it on linux. I could maybe rummage up a virtual machine and give it a go.

[alexfh]

In D18396#391031, @Rob wrote:

ok, I'm not sure if its worth rolling back the change to modernize-use-override-ms.cpp, see ammended comments above.

No, the test is fine as it is.

Apart from this it should be good to go.

I think I am going to need someone to submit the patch for me, I can't use svn due to 'error 400' so I've been using the git mirror.

If you don't know for sure, you don't have the commit access. I'll commit the patch for you.

test/clang-tidy/modernize-use-override-ms.cpp

3

Everything's fine with the test as it is now: both -fms-extensions and -std=c++11 are needed on non-windows platforms.

[alexfh]

The patch doesn't apply cleanly. Please rebase it on top of HEAD.

[alexfh]

In D18396#391084, @alexfh wrote:

The patch doesn't apply cleanly. Please rebase it on top of HEAD.

Actually, the conflict was only in the ReleaseNotes.rst file, which I had to change anyway to fix the formatting around the place you changed. The patch is committed as http://reviews.llvm.org/rL265298. Thank you!

[Rob]

Thanks :)

clang-tidy/modernize/UseOverrideCheck.cpp

if (OnlyVirtualSpecified) {
     Message = "annotate this function with 'override' or (rarely) 'final'";
   } else {
     StringRef Redundant =
         HasVirtual ? (HasOverride && HasFinal ? "'virtual' and 'override' are"
                                               : "'virtual' is")
                    : "'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);
 
   CharSourceRange FileRange = Lexer::makeFileCharRange(
       CharSourceRange::getTokenRange(Method->getSourceRange()), Sources,
       Result.Context->getLangOpts());
 
   if (!FileRange.isValid())
     return;
 
   // FIXME: Instead of re-lexing and looking for specific macros such as
   // 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each
   // FunctionDecl.
   SmallVector<Token, 16> Tokens = ParseTokens(FileRange, Result);
 
   // Add 'override' on inline declarations that don't already have it.
   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()) {
       // For methods with inline definition, add the override keyword at the
if (!InsertLoc.isValid()) {
       // For declarations marked with "= 0" or "= [default|delete]", the end
       // location will point until after those markings. Therefore, the override
       // keyword shouldn't be inserted at the end, but before the '='.
       if (Tokens.size() > 2 && (GetText(Tokens.back(), Sources) == "0" ||
                                 Tokens.back().is(tok::kw_default) ||
                                 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();
       }
     }
 
     if (!InsertLoc.isValid()) {
       InsertLoc = FileRange.getEnd();
       ReplacementText = " override";

test/clang-tidy/modernize-use-override-ms.cpp

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

test/clang-tidy/modernize-use-override.cpp

struct Base {
   virtual ~Base() {}
   virtual void a();
   virtual void b();
   virtual void c();
   virtual void d();
   virtual void d2();
   virtual void e() = 0;
   virtual void f() = 0;
+  virtual void f2() const = 0;
   virtual void g() = 0;
 
   virtual void j() const;
   virtual MustUseResultObject k();
   virtual bool l() MUST_USE_RESULT UNUSED;
   virtual bool n() MUST_USE_RESULT UNUSED;
 
   virtual void m();
public:
   // CHECK-FIXES: {{^}}  void d2() final;
 
   virtual void e() = 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
   // CHECK-FIXES: {{^}}  void e() override = 0;
 
   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
   // CHECK-FIXES: {{^}}  void g() override ABSTRACT;
 
   virtual void j() const;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
   // CHECK-FIXES: {{^}}  void j() const override;