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