[clang-tidy] Ignore forward declarations without definitions in the same translation unit in readability-identifier-naming

[jbcoe]

Summary

This change ensures that forward declarations of classes are not considered for identifier naming checks within a translation unit.

[aaron.ballman]

LGTM!

[mgehre]

I don't see how your fix relates to the 'header-filter' option. I can image the following cases:

1. a class is forward declared and then a system header (i.e. not in header-filter) is included that defines the class -> should be no warning
2. a class is forward declared and then a user header (i.e. in header-filter) is included that defines the class -> should be a warning
3. a class is forward declared and never defined -> should be no warning

It seems that you only consider point 3), correct?

[jbcoe]

@mgehre, you're right. I only consider "a class is forward declared and never defined -> should be no warning" and do not test for classes that are declared in excluded headers or system headers.

[alexfh]

Please include full diffs with the patch (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

[alexfh]

In D22571#493230, @mgehre wrote:

I don't see how your fix relates to the 'header-filter' option. I can image the following cases:

1. a class is forward declared and then a system header (i.e. not in header-filter) is included that defines the class -> should be no warning
2. a class is forward declared and then a user header (i.e. in header-filter) is included that defines the class -> should be a warning
3. a class is forward declared and never defined -> should be no warning

It seems that you only consider point 3), correct?

1 and 2 are a part of a more generic problem related to changes that need to be "transactional" (i.e. either all of the changes from a certain group - for example, renaming a certain identifier - need to be applied, or none). And we don't have a good solution to that yet. I don't think this can be properly solved exclusively on the level of the check, there should be some infrastructure support for this as well.

[jbcoe]

Upload full diff and amend summary.

clang-tidy/readability/IdentifierNamingCheck.cpp

if (isa<EnumConstantDecl>(D)) {
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<CXXRecordDecl>(D)) {
     if (Decl->isAnonymousStructOrUnion())
       return SK_Invalid;
 
+    if (!Decl->getCanonicalDecl()->isThisDeclarationADefinition())
+      return SK_Invalid;
+
     if (Decl->hasDefinition() && Decl->isAbstract() &&
         NamingStyles[SK_AbstractClass].isSet())
       return SK_AbstractClass;
 
     if (Decl->isStruct() && NamingStyles[SK_Struct].isSet())
       return SK_Struct;
 
     if (Decl->isStruct() && NamingStyles[SK_Class].isSet())

test/clang-tidy/readability-identifier-naming.cpp

// CHECK-FIXES: {{^}}    int pub_mem;{{$}}
 
     static const int classConstant;
 // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: invalid case style for class constant 'classConstant'
 // CHECK-FIXES: {{^}}    static const int kClassConstant;{{$}}
     static int ClassMember_2;
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for class member 'ClassMember_2'
 // CHECK-FIXES: {{^}}    static int ClassMember2;{{$}}
 };
+class my_class;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for class 'my_class'
+// CHECK-FIXES: {{^}}class CMyClass;{{$}}
+
+class my_forward_declared_class; // No warning should be triggered.
 
 const int my_class::classConstant = 4;
 // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: invalid case style for class constant 'classConstant'
 // CHECK-FIXES: {{^}}const int CMyClass::kClassConstant = 4;{{$}}
 
 int my_class::ClassMember_2 = 5;
 // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for class member 'ClassMember_2'
 // CHECK-FIXES: {{^}}int CMyClass::ClassMember2 = 5;{{$}}