[clang-tools-extra] r248996 - [clang-tidy] Implement FixitHints for identifier references in IdentifierNamingCheck
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 1 02:19:41 PDT 2015
Author: alexfh
Date: Thu Oct 1 04:19:40 2015
New Revision: 248996
URL: http://llvm.org/viewvc/llvm-project?rev=248996&view=rev
Log:
[clang-tidy] Implement FixitHints for identifier references in IdentifierNamingCheck
This diff requires http://reviews.llvm.org/D13079 to be applied first. I wasn't sure about how to make patch series in Phabricator, and I wanted to keep the two separate for clarity.
It looks like that most cases can be supported with this patch. I'm not totally sure about the actual coverage though. I think that the matchers are very generic, but I'm still not totally fluent with the AST.
Patch by Beren Minor!
Differential revision: http://reviews.llvm.org/D13081
Added:
clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/
clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/system/
clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/system/system-header.h
clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/user-header.h
Modified:
clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
Modified: clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp?rev=248996&r1=248995&r2=248996&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp Thu Oct 1 04:19:40 2015
@@ -136,13 +136,13 @@ void IdentifierNamingCheck::storeOptions
}
void IdentifierNamingCheck::registerMatchers(MatchFinder *Finder) {
- // FIXME: For now, only Decl and DeclRefExpr nodes are visited for checking
- // and replacement. There is a lot of missing cases, such as references to a
- // class name (as in 'const int CMyClass::kClassConstant = 4;'), to an
- // enclosing context (namespace, class, etc).
-
Finder->addMatcher(namedDecl().bind("decl"), this);
- Finder->addMatcher(declRefExpr().bind("declref"), this);
+ Finder->addMatcher(usingDecl().bind("using"), this);
+ Finder->addMatcher(declRefExpr().bind("declRef"), this);
+ Finder->addMatcher(cxxConstructorDecl().bind("classRef"), this);
+ Finder->addMatcher(cxxDestructorDecl().bind("classRef"), this);
+ Finder->addMatcher(typeLoc().bind("typeLoc"), this);
+ Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this);
}
static bool matchesStyle(StringRef Name,
@@ -504,27 +504,121 @@ static StyleKind findStyleKind(
static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures,
const NamedDecl *Decl, SourceRange Range,
const SourceManager *SM) {
+ // Do nothin if the provided range is invalid
+ if (Range.getBegin().isInvalid() || Range.getEnd().isInvalid())
+ return;
+
+ // Try to insert the identifier location in the Usages map, and bail out if it
+ // is already in there
auto &Failure = Failures[Decl];
if (!Failure.RawUsageLocs.insert(Range.getBegin().getRawEncoding()).second)
return;
- Failure.ShouldFix = Failure.ShouldFix && SM->isInMainFile(Range.getBegin()) &&
- SM->isInMainFile(Range.getEnd()) &&
- !Range.getBegin().isMacroID() &&
+ Failure.ShouldFix = Failure.ShouldFix && !Range.getBegin().isMacroID() &&
!Range.getEnd().isMacroID();
}
void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
- if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declref")) {
+ if (const auto *Decl =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) {
+ if (Decl->isImplicit())
+ return;
+
+ addUsage(NamingCheckFailures, Decl->getParent(),
+ Decl->getNameInfo().getSourceRange(), Result.SourceManager);
+ return;
+ }
+
+ if (const auto *Decl =
+ Result.Nodes.getNodeAs<CXXDestructorDecl>("classRef")) {
+ if (Decl->isImplicit())
+ return;
+
+ SourceRange Range = Decl->getNameInfo().getSourceRange();
+ if (Range.getBegin().isInvalid())
+ return;
+ // The first token that will be found is the ~ (or the equivalent trigraph),
+ // we want instead to replace the next token, that will be the identifier.
+ Range.setBegin(CharSourceRange::getTokenRange(Range).getEnd());
+
+ addUsage(NamingCheckFailures, Decl->getParent(), Range,
+ Result.SourceManager);
+ return;
+ }
+
+ if (const auto *Loc = Result.Nodes.getNodeAs<TypeLoc>("typeLoc")) {
+ NamedDecl *Decl = nullptr;
+ if (const auto &Ref = Loc->getAs<TagTypeLoc>()) {
+ Decl = Ref.getDecl();
+ } else if (const auto &Ref = Loc->getAs<InjectedClassNameTypeLoc>()) {
+ Decl = Ref.getDecl();
+ } else if (const auto &Ref = Loc->getAs<UnresolvedUsingTypeLoc>()) {
+ Decl = Ref.getDecl();
+ } else if (const auto &Ref = Loc->getAs<TemplateTypeParmTypeLoc>()) {
+ Decl = Ref.getDecl();
+ }
+
+ if (Decl) {
+ addUsage(NamingCheckFailures, Decl, Loc->getSourceRange(),
+ Result.SourceManager);
+ return;
+ }
+
+ if (const auto &Ref = Loc->getAs<TemplateSpecializationTypeLoc>()) {
+ const auto *Decl =
+ Ref.getTypePtr()->getTemplateName().getAsTemplateDecl();
+
+ SourceRange Range(Ref.getTemplateNameLoc(), Ref.getTemplateNameLoc());
+ if (const auto *ClassDecl = dyn_cast<TemplateDecl>(Decl)) {
+ addUsage(NamingCheckFailures, ClassDecl->getTemplatedDecl(), Range,
+ Result.SourceManager);
+ return;
+ }
+ }
+
+ if (const auto &Ref =
+ Loc->getAs<DependentTemplateSpecializationTypeLoc>()) {
+ addUsage(NamingCheckFailures, Ref.getTypePtr()->getAsTagDecl(),
+ Loc->getSourceRange(), Result.SourceManager);
+ return;
+ }
+ }
+
+ if (const auto *Loc =
+ Result.Nodes.getNodeAs<NestedNameSpecifierLoc>("nestedNameLoc")) {
+ if (NestedNameSpecifier *Spec = Loc->getNestedNameSpecifier()) {
+ if (NamespaceDecl *Decl = Spec->getAsNamespace()) {
+ addUsage(NamingCheckFailures, Decl, Loc->getLocalSourceRange(),
+ Result.SourceManager);
+ return;
+ }
+ }
+ }
+
+ if (const auto *Decl = Result.Nodes.getNodeAs<UsingDecl>("using")) {
+ for (const auto &Shadow : Decl->shadows()) {
+ addUsage(NamingCheckFailures, Shadow->getTargetDecl(),
+ Decl->getNameInfo().getSourceRange(), Result.SourceManager);
+ }
+ return;
+ }
+
+ if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
SourceRange Range = DeclRef->getNameInfo().getSourceRange();
- return addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
- Result.SourceManager);
+ addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
+ Result.SourceManager);
+ return;
}
if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
return;
+ // Ignore ClassTemplateSpecializationDecl which are creating duplicate
+ // replacements with CXXRecordDecl
+ if (isa<ClassTemplateSpecializationDecl>(Decl))
+ return;
+
StyleKind SK = findStyleKind(Decl, NamingStyles);
if (SK == SK_Invalid)
return;
@@ -566,13 +660,21 @@ void IdentifierNamingCheck::onEndOfTrans
if (Failure.KindName.empty())
continue;
- auto Diag = diag(Decl.getLocStart(), "invalid case style for %0 '%1'")
- << Failure.KindName << Decl.getName();
if (Failure.ShouldFix) {
+ auto Diag = diag(Decl.getLocStart(), "invalid case style for %0 '%1'")
+ << Failure.KindName << Decl.getName();
+
for (const auto &Loc : Failure.RawUsageLocs) {
// We assume that the identifier name is made of one token only. This is
// always the case as we ignore usages in macros that could build
// identifier names by combining multiple tokens.
+ //
+ // For destructors, we alread take care of it by remembering the
+ // location of the start of the identifier and not the start of the
+ // tilde.
+ //
+ // Other multi-token identifiers, such as operators are not checked at
+ // all.
Diag << FixItHint::CreateReplacement(
SourceRange(SourceLocation::getFromRawEncoding(Loc)),
Failure.Fixup);
Added: clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/system/system-header.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/system/system-header.h?rev=248996&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/system/system-header.h (added)
+++ clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/system/system-header.h Thu Oct 1 04:19:40 2015
@@ -0,0 +1,17 @@
+#ifndef SYSTEM_HEADER_H
+#define SYSTEM_HEADER_H
+
+#define SYSTEM_MACRO(m) int m = 0
+
+namespace SYSTEM_NS {
+class structure {
+ int member;
+};
+
+float global;
+
+void function() {
+}
+}
+
+#endif // SYSTEM_HEADER_H
Added: clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/user-header.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/user-header.h?rev=248996&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/user-header.h (added)
+++ clang-tools-extra/trunk/test/clang-tidy/Inputs/readability-identifier-naming/user-header.h Thu Oct 1 04:19:40 2015
@@ -0,0 +1,17 @@
+#ifndef USER_HEADER_H
+#define USER_HEADER_H
+
+#define USER_MACRO(m) int m = 0
+
+namespace USER_NS {
+class object {
+ int member;
+};
+
+float global;
+
+void function() {
+}
+}
+
+#endif // USER_HEADER_H
Modified: clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp?rev=248996&r1=248995&r2=248996&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp Thu Oct 1 04:19:40 2015
@@ -62,14 +62,17 @@
// RUN: {key: readability-identifier-naming.VirtualMethodCase, value: UPPER_CASE}, \
// RUN: {key: readability-identifier-naming.VirtualMethodPrefix, value: 'v_'}, \
// RUN: {key: readability-identifier-naming.IgnoreFailedSplit, value: 0} \
-// RUN: ]}' -- -std=c++11 -fno-delayed-template-parsing
-
-// FIXME: There should be more test cases for checking that references to class
-// FIXME: name, declaration contexts, forward declarations, etc, are correctly
-// FIXME: checked and renamed
+// RUN: ]}' -- -std=c++11 -fno-delayed-template-parsing \
+// RUN: -I%S/Inputs/readability-identifier-naming \
+// RUN: -isystem %S/Inputs/readability-identifier-naming/system
// clang-format off
+#include <system-header.h>
+#include "user-header.h"
+// NO warnings or fixes expected from declarations within header files without
+// the -header-filter= option
+
namespace FOO_NS {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for namespace 'FOO_NS' [readability-identifier-naming]
// CHECK-FIXES: {{^}}namespace foo_ns {{{$}}
@@ -77,10 +80,26 @@ inline namespace InlineNamespace {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for inline namespace 'InlineNamespace'
// CHECK-FIXES: {{^}}inline namespace inline_namespace {{{$}}
+SYSTEM_NS::structure g_s1;
+// NO warnings or fixes expected as SYSTEM_NS and structure are declared in a header file
+
+USER_NS::object g_s2;
+// NO warnings or fixes expected as USER_NS and object are declared in a header file
+
+SYSTEM_MACRO(var1);
+// NO warnings or fixes expected as var1 is from macro expansion
+
+USER_MACRO(var2);
+// NO warnings or fixes expected as var2 is declared in a macro expansion
+
+int global;
+#define USE_IN_MACRO(m) auto use_##m = m
+USE_IN_MACRO(global);
+// NO warnings or fixes expected as global is used in a macro expansion
+
#define BLA int FOO_bar
BLA;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for global variable 'FOO_bar'
-// NO fix expected as FOO_bar is from macro expansion
+// NO warnings or fixes expected as FOO_bar is from macro expansion
enum my_enumeration {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for enum 'my_enumeration'
@@ -104,6 +123,13 @@ class my_class {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_class'
// CHECK-FIXES: {{^}}class CMyClass {{{$}}
my_class();
+// CHECK-FIXES: {{^}} CMyClass();{{$}}
+
+ ~
+ my_class();
+// (space in destructor token test, we could check trigraph but they will be deprecated)
+// CHECK-FIXES: {{^}} ~{{$}}
+// CHECK-FIXES: {{^}} CMyClass();{{$}}
const int MEMBER_one_1 = ConstExpr_variable;
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: invalid case style for constant member 'MEMBER_one_1'
@@ -137,15 +163,36 @@ public:
const int my_class::classConstant = 4;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class constant 'classConstant'
-// CHECK-FIXES: {{^}}const int my_class::kClassConstant = 4;{{$}}
-// FIXME: The fixup should reflect class name fixups as well:
-// FIXME: {{^}}const int CMyClass::kClassConstant = 4;{{$}}
+// CHECK-FIXES: {{^}}const int CMyClass::kClassConstant = 4;{{$}}
int my_class::ClassMember_2 = 5;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class member 'ClassMember_2'
-// CHECK-FIXES: {{^}}int my_class::ClassMember2 = 5;{{$}}
-// FIXME: The fixup should reflect class name fixups as well:
-// FIXME: {{^}}int CMyClass::ClassMember2 = 5;{{$}}
+// CHECK-FIXES: {{^}}int CMyClass::ClassMember2 = 5;{{$}}
+
+class my_derived_class : public virtual my_class {};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_derived_class'
+// CHECK-FIXES: {{^}}class CMyDerivedClass : public virtual CMyClass {};{{$}}
+
+class CMyWellNamedClass {};
+// No warning expected as this class is well named.
+
+template<typename T>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for type template parameter 'T'
+// CHECK-FIXES: {{^}}template<typename t_t>{{$}}
+class my_templated_class : CMyWellNamedClass {};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_templated_class'
+// CHECK-FIXES: {{^}}class CMyTemplatedClass : CMyWellNamedClass {};{{$}}
+
+template<typename T>
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for type template parameter 'T'
+// CHECK-FIXES: {{^}}template<typename t_t>{{$}}
+class my_other_templated_class : my_templated_class< my_class>, private my_derived_class {};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for class 'my_other_templated_class'
+// CHECK-FIXES: {{^}}class CMyOtherTemplatedClass : CMyTemplatedClass< CMyClass>, private CMyDerivedClass {};{{$}}
+
+template<typename t_t>
+using MYSUPER_TPL = my_other_templated_class <:: FOO_NS ::my_class>;
+// CHECK-FIXES: {{^}}using MYSUPER_TPL = CMyOtherTemplatedClass <:: foo_ns ::CMyClass>;{{$}}
const int global_Constant = 6;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for global constant 'global_Constant'
@@ -186,7 +233,7 @@ template<typename ... TYPE_parameters>
void Global_Fun(TYPE_parameters... PARAMETER_PACK) {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for global function 'Global_Fun'
// CHECK-MESSAGES: :[[@LINE-2]]:17: warning: invalid case style for parameter pack 'PARAMETER_PACK'
-// CHECK-FIXES: {{^}}void GlobalFun(TYPE_parameters... parameterPack) {{{$}}
+// CHECK-FIXES: {{^}}void GlobalFun(typeParameters_t... parameterPack) {{{$}}
global_function(1, 2);
// CHECK-FIXES: {{^}} GlobalFunction(1, 2);{{$}}
FOO_bar = Global_variable;
@@ -208,12 +255,15 @@ class abstract_class {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for abstract class 'abstract_class'
// CHECK-FIXES: {{^}}class AAbstractClass {{{$}}
virtual ~abstract_class() = 0;
+// CHECK-FIXES: {{^}} virtual ~AAbstractClass() = 0;{{$}}
virtual void VIRTUAL_METHOD();
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for virtual method 'VIRTUAL_METHOD'
// CHECK-FIXES: {{^}} virtual void v_VIRTUAL_METHOD();{{$}}
void non_Virtual_METHOD() {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for private method 'non_Virtual_METHOD'
// CHECK-FIXES: {{^}} void __non_Virtual_METHOD() {}{{$}}
+
+public:
static void CLASS_METHOD() {}
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for class method 'CLASS_METHOD'
// CHECK-FIXES: {{^}} static void classMethod() {}{{$}}
@@ -244,8 +294,7 @@ struct THIS___Structure {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for struct 'THIS___Structure'
// CHECK-FIXES: {{^}}struct this_structure {{{$}}
THIS___Structure();
-// FIXME: There should be a fixup for the constructor as well
-// FIXME: {{^}} this_structure();{{$}}
+// CHECK-FIXES: {{^}} this_structure();{{$}}
union __MyUnion_is_wonderful__ {};
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: invalid case style for union '__MyUnion_is_wonderful__'
@@ -254,13 +303,19 @@ struct THIS___Structure {
typedef THIS___Structure struct_type;
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for typedef 'struct_type'
-// CHECK-FIXES: {{^}}typedef THIS___Structure struct_type_t;{{$}}
-// FIXME: The fixup should reflect structure name fixups as well:
-// FIXME: {{^}}typedef this_structure struct_type_t;{{$}}
+// CHECK-FIXES: {{^}}typedef this_structure struct_type_t;{{$}}
static void static_Function() {
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for function 'static_Function'
// CHECK-FIXES: {{^}}static void staticFunction() {{{$}}
+
+ ::FOO_NS::InlineNamespace::abstract_class::CLASS_METHOD();
+// CHECK-FIXES: {{^}} ::foo_ns::inline_namespace::AAbstractClass::classMethod();{{$}}
+ ::FOO_NS::InlineNamespace::static_Function();
+// CHECK-FIXES: {{^}} ::foo_ns::inline_namespace::staticFunction();{{$}}
+
+ using ::FOO_NS::InlineNamespace::CE_function;
+// CHECK-FIXES: {{^}} using ::foo_ns::inline_namespace::ce_function;{{$}}
}
}
More information about the cfe-commits
mailing list