[clang-tidy] Fix readability-identifier-naming missing member variables

[njames93]

Summary

Fixes (clang-tidy) readability-identifier-naming misses fixing member variables in destructor and readability-identifier-naming does not rename class members in all locations
I've tried to be pretty exhaustive with test cases, but there will always be some edge cases

[njames93]

More edge cases handled to do with field decls and initializers

[JonasToth]

Thank you for fixing these bugs!
Did you try running the check of clang/llvm again, to see if there are still missing cases of this?

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

571

these all seem to be an artifact of running clang-format?
Such a change can be done, but should happen in a separate patch (just formatting does not require review).

736

this change violates the coding guideline.

821

const auto * to show that this variable is a pointer.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

89

I dont understand this comment. What do you mean?

[njames93]

Ahh clang format script must have run, I'll undo that. I've not tried running it against the whole of llvm as I don't know what the easy way to do that is, especially given that llvm disregards it's standard naming convention for old projects and stl like containers. The comment is just in relation to the bug report that this fix is based on, could probably go

[JonasToth]

In D72121#1803242, @njames93 wrote:

Ahh clang format script must have run, I'll undo that. I've not tried running it against the whole of llvm as I don't know what the easy way to do that is, especially given that llvm disregards it's standard naming convention for old projects and stl like containers. The comment is just in relation to the bug report that this fix is based on, could probably go

yeah clang-format tends to do that, as devs do not consistently apply automatic formatting :/

for llvm: https://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
You can use run-clang-tidy.py in the tools/ directory of clang-tidy. Then specify your custom built clang-tidy binary.

The call should like similar to this (not tested):
<path-to-llvm>/llvm-project $ run-clang-tidy.py -p <path-to-build-dir-with-compile-commands> -clang-tidy-binary <path-to-build> -checks=-*,readability-identifier-naming -fix clang/lib

That will run over clang/libs only. If you choose lib/ it will run over each lib-subdirectory of the project.

Maybe you can still try it out and see what diagnostics you get. It will be a good practice anyway, if you plan to keep developing :)

[njames93]

addressed issues. Running the clang-tidy on the whole of lib clang just resulted in errors when compiling due to its dependancy on llvm, but I kind of knew that was going to happen

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

137

I think this is one more test case to add: base class specifiers. e.g.,

struct foo_bar {}; // Should be renamed

struct Baz : foo_bar {}; // Should rename foo_bar

[njames93]

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

137

That test case already exists in the readability-identifier-naming.cpp file

class my_derived_class : public virtual my_class {};
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for class 'my_derived_class'
// CHECK-FIXES: {{^}}class CMyDerivedClass : public virtual CMyClass {};{{$}}

[aaron.ballman]

LGTM!

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

137

Oh, thank you for pointing that out!

[njames93]

Is anyone able to commit on my behalf?

[aaron.ballman]

In D72121#1817170, @njames93 wrote:

Is anyone able to commit on my behalf?

Happy to do so. I've committed for you in fb79ef524171c96a9f3df025ac7a8a3e00fdc0b4

[Eugene.Zelenko]

Please also close PRs.

[njames93]

In D72121#1817733, @Eugene.Zelenko wrote:

Please also close PRs.

Done

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
 
   Options.store(Opts, "IgnoreFailedSplit", IgnoreFailedSplit);
 }
 
 void IdentifierNamingCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(namedDecl().bind("decl"), 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(cxxConstructorDecl(unless(isImplicit())).bind("classRef"),
+                     this);
+  Finder->addMatcher(cxxDestructorDecl(unless(isImplicit())).bind("classRef"),
+                     this);
   Finder->addMatcher(typeLoc().bind("typeLoc"), this);
   Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this);
+  Finder->addMatcher(
+      functionDecl(unless(cxxMethodDecl(isImplicit())),
+                   hasBody(forEachDescendant(memberExpr().bind("memberExpr")))),
+      this);
+  Finder->addMatcher(
+      cxxConstructorDecl(
+          unless(isImplicit()),
+          forEachConstructorInitializer(
+              allOf(isWritten(), withInitializer(forEachDescendant(
+                                     memberExpr().bind("memberExpr")))))),
+      this);
+  Finder->addMatcher(fieldDecl(hasInClassInitializer(
+                         forEachDescendant(memberExpr().bind("memberExpr")))),
+                     this);
 }
 
 void IdentifierNamingCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   ModuleExpanderPP->addPPCallbacks(
       std::make_unique<IdentifierNamingCheckPPCallbacks>(ModuleExpanderPP,
                                                           this));
 }
if (!Type.isNull() && Type.isConstQualified()) {
 
       if (NamingStyles[SK_Constant])
         return SK_Constant;
     }
 
     if (Decl->isStaticDataMember() && NamingStyles[SK_ClassMember])
       return SK_ClassMember;
 
     if (Decl->isFileVarDecl() && Type.getTypePtr()->isAnyPointerType() && NamingStyles[SK_GlobalPointer])
       return SK_GlobalPointer;
 
     if (Decl->isFileVarDecl() && NamingStyles[SK_GlobalVariable])
       return SK_GlobalVariable;
 
     if (Decl->isStaticLocal() && NamingStyles[SK_StaticVariable])
       return SK_StaticVariable;
  
return addUsage(Failures,
                   IdentifierNamingCheck::NamingCheckId(Decl->getLocation(),
                                                        Decl->getNameAsString()),
                   Range, SourceMgr);
 }
 
 void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Decl =
           Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) {
-    if (Decl->isImplicit())
-      return;
 
     addUsage(NamingCheckFailures, Decl->getParent(),
              Decl->getNameInfo().getSourceRange());
 
     for (const auto *Init : Decl->inits()) {
       if (!Init->isWritten() || Init->isInClassMemberInitializer())
         continue;
       if (const auto *FD = Init->getAnyMember())
         addUsage(NamingCheckFailures, FD,
                  SourceRange(Init->getMemberLocation()));
       // Note: delegating constructors and base class initializers are handled
       // via the "typeLoc" matcher.
     }
     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());
 
void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
 
   if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
     SourceRange Range = DeclRef->getNameInfo().getSourceRange();
     addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
              Result.SourceManager);
     return;
   }
 
+  if (const auto *MemberRef =
+          Result.Nodes.getNodeAs<MemberExpr>("memberExpr")) {
+    SourceRange Range = MemberRef->getMemberNameInfo().getSourceRange();
+    addUsage(NamingCheckFailures, MemberRef->getMemberDecl(), Range,
+             Result.SourceManager);
+    return;
+  }
+
   if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
       return;
 
     // Fix type aliases in value declarations
     if (const auto *Value = Result.Nodes.getNodeAs<ValueDecl>("decl")) {
       if (const auto *TypePtr = Value->getType().getTypePtrOrNull()) {
         if (const auto *Typedef = TypePtr->getAs<TypedefType>()) {

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: readability-identifier-naming.MemberCase, value: CamelCase}, \
+// RUN:     {key: readability-identifier-naming.ParameterCase, value: CamelCase} \
+// RUN:  ]}'
+
+int set_up(int);
+int clear(int);
+
+class Foo {
+public:
+  const int bar_baz; // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for member 'bar_baz'
+  // CHECK-FIXES: {{^}}  const int BarBaz; // comment-0
+
+  Foo(int Val) : bar_baz(Val) { // comment-1
+    // CHECK-FIXES: {{^}}  Foo(int Val) : BarBaz(Val) { // comment-1
+    set_up(bar_baz); // comment-2
+    // CHECK-FIXES: {{^}}    set_up(BarBaz); // comment-2
+  }
+
+  Foo() : Foo(0) {}
+
+  ~Foo() {
+    clear(bar_baz); // comment-3
+    // CHECK-FIXES: {{^}}    clear(BarBaz); // comment-3
+  }
+
+  int getBar() const { return bar_baz; } // comment-4
+  // CHECK-FIXES: {{^}}  int getBar() const { return BarBaz; } // comment-4
+};
+
+class FooBar : public Foo {
+public:
+  int getFancyBar() const {
+    return this->bar_baz; // comment-5
+    // CHECK-FIXES: {{^}}    return this->BarBaz; // comment-5
+  }
+};
+
+int getBar(const Foo &Foo) {
+  return Foo.bar_baz; // comment-6
+  // CHECK-FIXES: {{^}}  return Foo.BarBaz; // comment-6
+}
+
+int getBar(const FooBar &Foobar) {
+  return Foobar.bar_baz; // comment-7
+  // CHECK-FIXES: {{^}}  return Foobar.BarBaz; // comment-7
+}
+
+int getFancyBar(const FooBar &Foobar) {
+  return Foobar.getFancyBar();
+}
+
+template <typename Dummy>
+class TempTest : public Foo {
+public:
+  TempTest() = default;
+  TempTest(int Val) : Foo(Val) {}
+  int getBar() const { return Foo::bar_baz; } // comment-8
+  // CHECK-FIXES: {{^}}  int getBar() const { return Foo::BarBaz; } // comment-8
+  int getBar2() const { return this->bar_baz; } // comment-9
+  // CHECK-FIXES: {{^}}  int getBar2() const { return this->BarBaz; } // comment-9
+};
+
+TempTest<int> x; //force an instantiation
+
+int blah() {
+  return x.getBar2(); // gotta have a reference to the getBar2 so that the
+                      // compiler will generate the function and resolve
+                      // the DependantScopeMemberExpr
+}
+
+namespace Bug41122 {
+namespace std {
+
+// for this example we aren't bothered about how std::vector is treated
+template <typename T> //NOLINT
+class vector { //NOLINT
+public:
+  void push_back(bool); //NOLINT
+  void pop_back(); //NOLINT
+}; //NOLINT
+}; // namespace std
+
+class Foo { 
+  std::vector<bool> &stack;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: invalid case style for member 'stack' [readability-identifier-naming]
+public:
+  Foo(std::vector<bool> &stack)
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: invalid case style for parameter 'stack' [readability-identifier-naming]
+      // CHECK-FIXES: {{^}}  Foo(std::vector<bool> &Stack)
+      : stack(stack) {
+    // CHECK-FIXES: {{^}}      : Stack(Stack) {
+    stack.push_back(true);
+    // CHECK-FIXES: {{^}}    Stack.push_back(true);
+  }
+  ~Foo() {
+    stack.pop_back();
+    // CHECK-FIXES: {{^}}    Stack.pop_back();
+  }
+};
+}; // namespace Bug41122
+
+namespace Bug29005 {
+class Foo {
+public:
+  int a_member_of_foo = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'a_member_of_foo'
+  // CHECK-FIXES: {{^}}  int AMemberOfFoo = 0;
+};
+
+int main() {
+  Foo foo;
+  return foo.a_member_of_foo;
+  // CHECK-FIXES: {{^}}  return foo.AMemberOfFoo;
+}
+}; // namespace Bug29005
+
+namespace CtorInits {
+template <typename T, unsigned N>
+class Container {
+  T storage[N];
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for member 'storage'
+  // CHECK-FIXES: {{^}}  T Storage[N];
+  T *pointer = &storage[0];
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for member 'pointer'
+  // CHECK-FIXES: {{^}}  T *Pointer = &Storage[0];
+public:
+  Container() : pointer(&storage[0]) {}
+  // CHECK-FIXES: {{^}}  Container() : Pointer(&Storage[0]) {}
+};
+
+void foo() {
+  Container<int, 5> container;
+}
+}; // namespace CtorInits