clang-tidy modernize-use-override
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 22 07:16:22 PDT 2016
On Mon, Mar 21, 2016 at 11:22 AM, Robert Bolter via cfe-commits
<cfe-commits at 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.bolter at 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
More information about the cfe-commits
mailing list