Skip to content

Commit

Permalink
[clang-tidy] Implement FixitHints for identifier references in Identi…
Browse files Browse the repository at this point in the history 
…fierNamingCheck

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

llvm-svn: 248996
  • Loading branch information
@alexfh
alexfh committed Oct 1, 2015
1 parent 746ffd6 commit 30c423b
Show file tree
Hide file tree
Showing 4 changed files with 224 additions and 33 deletions.
130 changes: 116 additions & 14 deletions clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
View file
Expand Up @@ -136,13 +136,13 @@ void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
}

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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -566,13 +660,21 @@ void IdentifierNamingCheck::onEndOfTranslationUnit() {
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);
Expand Down
17 changes: 17 additions & 0 deletions ...g-tools-extra/test/clang-tidy/Inputs/readability-identifier-naming/system/system-header.h
View file
@@ -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
17 changes: 17 additions & 0 deletions clang-tools-extra/test/clang-tidy/Inputs/readability-identifier-naming/user-header.h
View file
@@ -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
93 changes: 74 additions & 19 deletions clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
View file
Expand Up @@ -62,25 +62,44 @@
// 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 {{{$}}
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'
Expand All @@ -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'
Expand Down Expand Up @@ -137,15 +163,36 @@ class my_class {

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'
Expand Down Expand Up @@ -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;
Expand All @@ -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() {}{{$}}
Expand Down Expand Up @@ -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__'
Expand All @@ -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;{{$}}
}

}
Expand Down

0 comments on commit 30c423b

Please sign in to comment.