[PATCH] D10933: Add new IdentifierNaming check

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:178
@@ +177,3 @@
+    if (NamingStyles[Typedef].isSet()) {
+      KindName = "typedef";
+      Style = NamingStyles[Typedef];
----------------
berenm wrote:
> alexfh wrote:
> > berenm wrote:
> > > alexfh wrote:
> > > > Would it be better to have these in a single array next to `StyleNames`? Also, this code could return a `StyleKind` (now `StyleConst`), and you would need to get the corresponding `NamingStyle` and `KindName` once.
> > > > 
> > > > This code would suddenly become much leaner.
> > > The problem is that sometimes, the current code falls back to a more generic naming style, but the "kind name" is still trying to describe the original declaration.
> > > 
> > > For example, if no style is defined for methods, then it will try to use a more generic "function" style, but the warning will still be "invalid case style for method xxx".
> > > 
> > > Maybe this is superfluous and I can drop it. It don't see many cases anyway (my original code was covering more cases - too many - and it seemed sensible at that time).
> > > The problem is that sometimes, the current code falls back to a more 
> > > generic naming style, but the "kind name" is still trying to describe the 
> > > original declaration.
> > 
> > I see. It might be possible to split the mapping of types to style kinds and handling missing styles. E.g. a function can return "Method" (which should be SK_Method according to [1], btw) and then a caller would check whether the corresponding `NamingStyle` is configured, and if needed fall back to a more generic category. WDYT?
> > 
> > 
> > [1] http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> I think it would be difficult to map the declaration types to style kinds without checking the styles that have been provided and falling back to another style at the same time.
> 
> For example, a constexpr method is currently selecting the styles in this preferred order :
> constexpr method > [public/protected/private] method > method > function.
> 
> The order is debatable, but we cannot have the //constexpr method// to //public/protected/private method// fallback, if there is no style for constexpr methods, without reading again the method declaration.
> 
> It might be OK that the warning message do not use the exact identifier kind name, and one can even think it is better to have a warning message that tells the user which style it used instead of which kind of identifier was checked.
> we cannot have the constexpr method to public/protected/private method fallback, if there is no style for 
> constexpr methods, without reading again the method declaration.

So the issue is caused by the fact that the categories are partially overlapping and don't form a hierarchy. Not sure whether this is convenient for the end-user and if it's a good model, but this can fit into the approach: the function can return an ordered list of `StyleKind`s that could then be looked up in the configured `NamingStyle`s. I don't insist on this specific implementation, but it might end up being more elegant solution.


http://reviews.llvm.org/D10933