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

2

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

2

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

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

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

docs/ReleaseNotes.rst

 :program:`clang-tidy`'s checks are constantly being improved to catch more issues,
 explain them more clearly, and provide more accurate fix-its for the issues
 identified.  The improvements since the 3.8 release include:
 
 - New ``modernize-raw-string-literal`` check
 
   This check selectively replaces string literals containing escaped
   characters with raw string literals.
   
+- Improved ``modernize-use-override`` check
+
+  The fix-its placement around __declspec attributes on windows and other 
+  attributes is improved.
+  
+
 Clang-tidy changes from 3.7 to 3.8
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 The 3.8 release didn't include release notes for :program:`clang-tidy`. In the
 3.8 release many new checks have been added to :program:`clang-tidy`:
 
 - Checks enforcing certain rules of the `CERT Secure Coding Standards
   <https://www.securecoding.cert.org/confluence/display/seccode/SEI+CERT+Coding+Standards>`_:

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

+// RUN: %check_clang_tidy %s modernize-use-override %t -- -- -fms-extensions
+
+// This test is designed to test ms-extension __declspec(dllexport) attributes.
+// On non-windows platforms these are not available so we test visibility attributes instead.
+#if defined(_MSC_VER)
+#  define EXPORT __declspec(dllexport)
+#else
+#  define EXPORT __attribute__((visibility("default")))
+#endif
+
+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;
+};
+

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;