[PATCH] D10933: Add new IdentifierNaming check
Alexander Kornienko
alexfh at google.com
Wed Aug 5 06:50:11 PDT 2015
alexfh added a comment.
Some initial comments, mostly style-related. It takes some time to get used to the coding style used in LLVM/Clang. One notable thing: though we use `auto` sometimes, we don't use the almost-always-auto style. Please see the inline comments for specific cases.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:63
@@ +62,3 @@
+
+enum StyleConst {
+#define ENUMERATE(v) v,
----------------
I'd name this `StyleKind` as most other enums in the LLVM code.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:69
@@ +68,3 @@
+
+static StyleConst const StyleKeys[] = {
+#define ENUMERATE(v) v,
----------------
The only use of this array seems to be a simplification of a single loop (the other one can iterate over `StyleNames` as per the comment below). I'd remove it and instead added a "StyleKindCount" element in the enum and use a normal for loop.
================
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;
----------------
This could be written nicer using llvm::StringSwitch.
================
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:
> 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.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:105
@@ +104,3 @@
+
+ for (const auto &Key : StyleKeys) {
+ NamingStyles.push_back(NamingStyle(
----------------
Why not iterate over `StyleNames` here?
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:151
@@ +150,3 @@
+ if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declref")) {
+ auto It = NamingCheckFailures.find(cast<NamedDecl>(DeclRef->getDecl()));
+ if (It == NamingCheckFailures.end())
----------------
No need to cast `ValueDecl` returned by `DeclRefExpr::getDecl()` to its base class.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:173
@@ +172,3 @@
+
+ auto KindName = "identifier";
+ auto Style = NamingStyle();
----------------
Please move the detection of the `KindName` and `Style` to a separate function. This method is too large.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:176
@@ +175,3 @@
+
+ if (Result.Nodes.getNodeAs<TypedefDecl>("decl")) {
+ if (NamingStyles[Typedef].isSet()) {
----------------
Please resolve the node only once (as it's done on line 168) and then just check it using `isa<...>(...)`.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:178
@@ +177,3 @@
+ if (NamingStyles[Typedef].isSet()) {
+ KindName = "typedef";
+ Style = NamingStyles[Typedef];
----------------
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.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:190
@@ +189,3 @@
+ Style = NamingStyles[InlineNamespace];
+
+ } else if (NamingStyles[Namespace].isSet()) {
----------------
Please remove empty lines before `}`.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:475
@@ +474,3 @@
+
+ auto matchesStyle = [](StringRef Name, NamingStyle Style) {
+ static llvm::Regex Matchers[] = {
----------------
Please make this a function. This method is too large to define every utility function inside it.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:477
@@ +476,3 @@
+ static llvm::Regex Matchers[] = {
+ llvm::Regex(StringRef("^.*$")),
+ llvm::Regex(StringRef("^[a-z][a-z0-9_]*$")),
----------------
No need to explicitly convert to `StringRef` here as well.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:505
@@ +504,3 @@
+
+ auto fixupWithStyle = [](std::string Name, NamingStyle Style) {
+ static auto Splitter = llvm::Regex(StringRef(
----------------
Please make this a function. This method is too large to define every utility function inside it.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:505
@@ +504,3 @@
+
+ auto fixupWithStyle = [](std::string Name, NamingStyle Style) {
+ static auto Splitter = llvm::Regex(StringRef(
----------------
alexfh wrote:
> Please make this a function. This method is too large to define every utility function inside it.
Please make `Name` a StringRef.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:506
@@ +505,3 @@
+ auto fixupWithStyle = [](std::string Name, NamingStyle Style) {
+ static auto Splitter = llvm::Regex(StringRef(
+ "(([a-z0-9A-Z]*)(_+)|([A-Z]?[a-z0-9]+)([A-Z]|$)|([A-Z]+)([A-Z]|$))"));
----------------
This doesn't look like a valid use case for `auto`. `static llvm::Regex Splitter(...);` is shorter and more readable.
Same applies to `Words`, `Substrs` and `Groups` below.
Also, you don't need to explicitly create a `StringRef`, just leave the string literal.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:509
@@ +508,3 @@
+
+ auto Words = SmallVector<std::string, 8>();
+ auto Substrs = SmallVector<StringRef, 8>();
----------------
Why not a `SmallVector<StringRef,...>`?
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:512
@@ +511,3 @@
+ StringRef(Name).split(Substrs, "_", -1, false);
+ for (std::string Substr : Substrs) {
+ while (!Substr.empty()) {
----------------
I think, this may be a `StringRef`.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:519
@@ +518,3 @@
+ if (Groups[3].size() > 0) {
+ Words.emplace_back(std::move(Groups[2]));
+ Substr = Substr.substr(Groups[0].size());
----------------
The use of `std::move` here doesn't make sense to me: `Groups[...]` is a `StringRef`, which isn't moved faster than it is copied.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:581
@@ +580,3 @@
+
+ auto Name = Decl->getName();
+ auto Fixup = fixupWithStyle(Name, Style);
----------------
We use `auto` when the type is spelled in the initializer and in a few other cases, e.g. when iterating over a map, and the element type is somewhat tricky to spell correctly. In other cases please use concrete types.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:607
@@ +606,3 @@
+ for (const auto &P : NamingCheckFailures) {
+ auto DeclRange =
+ clang::DeclarationNameInfo(P.first->getDeclName(),
----------------
`clang::DeclarationNameInfo DeclRange(...)` is better, IMO.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:610
@@ +609,3 @@
+ P.first->getLocation()).getSourceRange();
+ auto Diagn = diag(P.first->getLocStart(), "invalid case style for %0 '%1'")
+ << P.second.KindName << P.first->getName();
----------------
nit: `Diagn` is not a common abbreviation in clang-tidy code. `Diag` would be better.
================
Comment at: clang-tidy/readability/IdentifierNamingCheck.h:42
@@ +41,3 @@
+ NamingStyle(CaseType Case, std::string Prefix, std::string Suffix)
+ : Case(Case), Prefix(std::move(Prefix)), Suffix(std::move(Suffix)) {}
+
----------------
I don't see what you gain by using `std::move` here. How about changing the type of `Prefix` and `Suffix` to `const std::string &` and getting rid of `std::move`? That's a more common pattern.
================
Comment at: test/clang-tidy/readability-identifier-naming.cpp:1
@@ +1,2 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-identifier-naming %t -config='{CheckOptions: [{key: readability-identifier-naming.AbstractCase, value: CamelCase}, {key: readability-identifier-naming.AbstractPrefix, value: 'A'}, {key: readability-identifier-naming.ClassCase, value: CamelCase}, {key: readability-identifier-naming.ClassPrefix, value: 'C'}, {key: readability-identifier-naming.ClassConstantCase, value: CamelCase}, {key: readability-identifier-naming.ClassConstantPrefix, value: 'k'}, {key: readability-identifier-naming.ClassMemberCase, value: CamelCase}, {key: readability-identifier-naming.ClassMethodCase, value: camelBack}, {key: readability-identifier-naming.ConstantCase, value: UPPER_CASE}, {key: readability-identifier-naming.ConstantSuffix, value: '_CST'}, {key: readability-identifier-naming.ConstexprFunctionCase, value: lower_case}, {key: readability-identifier-naming.ConstexprMethodCase, value: lower_case}, {key: readability-identifier-naming.ConstexprVariableCase, value: lower_case}, {key: readability-identifier-naming.EnumCase, value: CamelCase}, {key: readability-identifier-naming.EnumPrefix, value: 'E'}, {key: readability-identifier-naming.EnumConstantCase, value: UPPER_CASE}, {key: readability-identifier-naming.FunctionCase, value: camelBack}, {key: readability-identifier-naming.GlobalConstantCase, value: UPPER_CASE}, {key: readability-identifier-naming.GlobalFunctionCase, value: CamelCase}, {key: readability-identifier-naming.GlobalVariableCase, value: lower_case}, {key: readability-identifier-naming.GlobalVariablePrefix, value: 'g_'}, {key: readability-identifier-naming.InlineNamespaceCase, value: lower_case}, {key: readability-identifier-naming.LocalConstantCase, value: CamelCase}, {key: readability-identifier-naming.LocalConstantPrefix, value: 'k'}, {key: readability-identifier-naming.LocalVariableCase, value: lower_case}, {key: readability-identifier-naming.MemberCase, value: CamelCase}, {key: readability-identifier-naming.MemberPrefix, value: 'm_'}, {key: readability-identifier-naming.MemberConstantCase, value: lower_case}, {key: readability-identifier-naming.MemberPrivatePrefix, value: '__'}, {key: readability-identifier-naming.MemberProtectedPrefix, value: '_'}, {key: readability-identifier-naming.MemberPublicCase, value: lower_case}, {key: readability-identifier-naming.MethodCase, value: camelBack}, {key: readability-identifier-naming.MethodPrivatePrefix, value: '__'}, {key: readability-identifier-naming.MethodProtectedPrefix, value: '_'}, {key: readability-identifier-naming.NamespaceCase, value: lower_case}, {key: readability-identifier-naming.ParameterCase, value: camelBack}, {key: readability-identifier-naming.ParameterPrefix, value: 'a_'}, {key: readability-identifier-naming.ParameterConstantCase, value: camelBack}, {key: readability-identifier-naming.ParameterConstantPrefix, value: 'i_'}, {key: readability-identifier-naming.ParameterPackCase, value: camelBack}, {key: readability-identifier-naming.PureFunctionCase, value: lower_case}, {key: readability-identifier-naming.PureMethodCase, value: camelBack}, {key: readability-identifier-naming.StaticConstantCase, value: UPPER_CASE}, {key: readability-identifier-naming.StaticVariableCase, value: camelBack}, {key: readability-identifier-naming.StaticVariablePrefix, value: 's_'}, {key: readability-identifier-naming.StructCase, value: lower_case}, {key: readability-identifier-naming.TemplateParameterCase, value: UPPER_CASE}, {key: readability-identifier-naming.TemplateTemplateParameterCase, value: CamelCase}, {key: readability-identifier-naming.TemplateUsingCase, value: lower_case}, {key: readability-identifier-naming.TemplateUsingPrefix, value: 'u_'}, {key: readability-identifier-naming.TypeTemplateParameterCase, value: camelBack}, {key: readability-identifier-naming.TypeTemplateParameterSuffix, value: '_t'}, {key: readability-identifier-naming.TypedefCase, value: lower_case}, {key: readability-identifier-naming.TypedefSuffix, value: '_t'}, {key: readability-identifier-naming.UnionCase, value: CamelCase}, {key: readability-identifier-naming.UnionPrefix, value: 'U'}, {key: readability-identifier-naming.UsingCase, value: lower_case}, {key: readability-identifier-naming.ValueTemplateParameterCase, value: camelBack}, {key: readability-identifier-naming.VariableCase, value: lower_case}, {key: readability-identifier-naming.VirtualMethodCase, value: UPPER_CASE}, {key: readability-identifier-naming.VirtualMethodPrefix, value: 'v_'}, {key: readability-identifier-naming.IgnoreFailedSplit, value: 0}]}' -- -std=c++11
+// REQUIRES: shell
----------------
I recently learned that RUN: lines can be split:
```
// RUN: some-long-command ... \
// RUN: -command-continues ... \
// RUN: -command-continues ... \
// RUN: -command-continues ... \
// RUN: -command-continues ... \
// RUN: finally-done
```
================
Comment at: test/clang-tidy/readability-identifier-naming.cpp:8
@@ +7,3 @@
+inline namespace InlineNamespace {
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for inline namespace 'InlineNamespace' [readability-identifier-naming]
+// CHECK-FIXES: inline_namespace
----------------
No need to repeat the check name in each message (` [readability-identifier-naming]`). It's enough to have it in the first CHECK-MESSAGES line. All other CHECK-MESSAGES lines can omit it.
http://reviews.llvm.org/D10933
More information about the cfe-commits
mailing list