[PATCH] D10933: Add new IdentifierNaming check

[Beren Minor]

berenm added a comment.

Thanks for the review, I will fix the various style issues and come back with an updated version.


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:87
@@ +86,3 @@
+  auto const fromString = [](StringRef Str) {
+    if (Str.equals("any") || Str.equals("aNy_CasE"))
+      return AnyCase;
----------------
alexfh wrote:
> alexfh wrote:
> > This could be written nicer using llvm::StringSwitch.
> Not sure why we would need alternative spellings of these options. That seems to just add complexity with no benefit.
The idea was to provide a way easier to type than the full names. This may not be very useful, and maybe a single simple name for each case scheme would be enough.

================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:178
@@ +177,3 @@
+    if (NamingStyles[Typedef].isSet()) {
+      KindName = "typedef";
+      Style = NamingStyles[Typedef];
----------------
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).

================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:512
@@ +511,3 @@
+      StringRef(Name).split(Substrs, "_", -1, false);
+      for (std::string Substr : Substrs) {
+        while (!Substr.empty()) {
----------------
alexfh wrote:
> I think, this may be a `StringRef`.
I think I had some issues when trying to use StringRef around here, I will investigate a bit more.


http://reviews.llvm.org/D10933