[PATCH] D10933: Add new IdentifierNaming check

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D10933#225696, @berenm wrote:

> Indeed, I probably don't have rights on Clang/LLVM repositories.
>
> I have updated the patch with stricter tests - and found a missing break statement (`clang-tidy/readability/IdentifierNamingCheck.cpp:201`).


Behold the magic power of testing ;)

> I also realized that the DeclRefExpr matcher used to find references to declared identifiers is only matching references to values and not to classes.

> 

> For example, when referencing the class in the out-of-line class static member declaration (`test/clang-tidy/readability-identifier-naming.cpp:133` and `test/clang-tidy/readability-identifier-naming.cpp:136`), or  when using a class identifier as the type of a typedef, (`test/clang-tidy/readability-identifier-naming.cpp:243`). Or even when declaring a variable of a type whose name must be fixed.

> 

> Is there any matcher that could be used for finding references to a TypeDecl? Should I go through calls to `getDeclContext`, `getType`, or `getUnderlyingType` depending on the context? Any easier way I don't know about?


Yep, renaming classes is not as trivial as it might seem. You'll need to look for `TypeLoc`s (the `loc()` matcher) of `QualType`s and `NestedNameSpecifier`s referring to the old class name. You'd also need to match constructor declarations and `UsingDecl`s separately.

Here's a rough list of matchers that search for the name of a class:

  loc(qualType(hasDeclaration(namedDecl(hasName(old_class)))))
  loc(nestedNameSpecifier(specifiesType(
                hasDeclaration(recordDecl(hasName(old_class))))))
  usingDecl(hasAnyUsingShadowDecl(hasTargetDecl(hasName(old_class))))
  recordDecl(hasName(old_class))
  constructorDecl(ofClass(hasName(old_class)))

I suggest that you add a bunch of FIXMEs in the test and we can iterate on the check in follow-up patches. Similar issues apply to namespace and function renaming.

What needs to be tested:

- for classes: forward declarations, definitions, constructors, destructors, using declarations, type aliases, use of the class name in `new`, casting, qualified names (Class::member) (also resulting from injected class name usage - `Class::Class::Class`), use of the class name in templates and macros (to ensure there are no duplicate or incorrect fixes), and probably something else that I forgot.
- for functions/methods: declarations, calls, using declarations, taking address of, with qualified names and without, probably something else.
- for namespaces: use in qualified names and using declarations and directives.


http://reviews.llvm.org/D10933