On Mon, Mar 21, 2016 at 11:22 AM, Robert Bolter via cfe-commits <cfe-commits@lists.llvm.org> wrote: > Hi, > > First time poster here, Please advise…
Thank you for the patches, and welcome! One piece of advice: it's easier for code reviewers to review unrelated patches in separate email chains. Also, if you use Phab instead of email, it makes tracking the code reviews a bit easier (but is not a requirement for submitting patches); you can find more information at: http://llvm.org/docs/Phabricator.html It's also helpful to CC reviewers directly when submitting a patch, if you can figure out who to add (and if you don't know, it's fine to simply mention that in the patch submission, the community can help find reviewers). I've CCed a few reviewers that do excellent work on clang-tidy. Patch review below. > From 1cebd57efd542b010ac9c9da54e3ab2e097beb33 Mon Sep 17 00:00:00 2001 > From: Rob <robert.bol...@autodesk.com> > Date: Mon, 21 Mar 2016 12:14:26 +0000 > Subject: [PATCH] modernize-use-override fix for const=0 (without spaces) and > __declspec(dll_export) attributes > > --- > clang-tidy/modernize/UseOverrideCheck.cpp | 17 +++++++--- > test/clang-tidy/modernize-use-override-ms.cpp | 45 > +++++++++++++++++++++++++++ > test/clang-tidy/modernize-use-override.cpp | 7 ++++- > 3 files changed, 64 insertions(+), 5 deletions(-) > create mode 100644 test/clang-tidy/modernize-use-override-ms.cpp > > diff --git a/clang-tidy/modernize/UseOverrideCheck.cpp > b/clang-tidy/modernize/UseOverrideCheck.cpp > index a335748..acc0b4a 100644 > --- a/clang-tidy/modernize/UseOverrideCheck.cpp > +++ b/clang-tidy/modernize/UseOverrideCheck.cpp > @@ -118,9 +118,11 @@ void UseOverrideCheck::check(const > MatchFinder::MatchResult &Result) { > 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,12 +130,15 @@ void UseOverrideCheck::check(const > MatchFinder::MatchResult &Result) { > > 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)) > - InsertLoc = Loc; > + Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) { > + // skip attributes that precede the method identifier Comments should be complete sentences (properly capitalized and punctuated). > + if (!Sources.isBeforeInTranslationUnit(Loc, MethodLoc)) > + InsertLoc = Loc; > + } I think you can combine these if statements into: if ((InsertLoc.isInvalid() || Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) && !Sources.isBeforeInTranslationUnit(Loc, MethodLoc)) InsertLoc = Loc; > } > } > } > @@ -163,6 +168,10 @@ void UseOverrideCheck::check(const > MatchFinder::MatchResult &Result) { > 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 Same comments about comments here. > + if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == > 0) { > + ReplacementText = " override "; > + } I think it would be more clear for the replacement text assignment be moved below, where ReplacementText is being set to = " override". > } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") { > InsertLoc = Tokens.back().getLocation(); > } > diff --git a/test/clang-tidy/modernize-use-override-ms.cpp > b/test/clang-tidy/modernize-use-override-ms.cpp > new file mode 100644 > index 0000000..7ec56cd > --- /dev/null > +++ b/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 Please use the entire warning diagnostic text the first time it appears in the test file. The rest of the shortened uses are fine. > + // 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; > +}; > diff --git a/test/clang-tidy/modernize-use-override.cpp > b/test/clang-tidy/modernize-use-override.cpp > index e4be332..1e39e37 100644 > --- a/test/clang-tidy/modernize-use-override.cpp > +++ b/test/clang-tidy/modernize-use-override.cpp > @@ -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 > -- > 1.9.5.msysgit.1 I'm not really qualified to review your second patch, so I don't have any comments on it. ~Aaron _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
