[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name

[Firat Kasmis via Phabricator via cfe-commits]

firolino marked 18 inline comments as done.
firolino added a comment.

Addressed suggestions from @aaron.ballman Thanks!



================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:22
+
+const internal::VariadicDynCastAllOfMatcher<Decl, TagDecl> tagDecl;
+
----------------
aaron.ballman wrote:
> We may want to consider adding this to ASTMatchers.h at some point (can be done in a future patch).
Is there a `// FIXME: Once I am in AstMatcher.h, remove me.` OK?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:25
+static bool isWhitespaceExceptNL(unsigned char C);
+static std::string removeMultiLineComments(std::string Str);
+static std::string getCurrentLineIndent(SourceLocation Loc,
----------------
aaron.ballman wrote:
> This should accept by `const std::string &`.
Since I need a copy, it is more efficient to pass-by-value instead pass-by-reference + copy.  So, I get a benefit for rvalues and move will be applied. Search for "want speed? pass by value!" articles.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:34
+  // a tag declaration (e.g. struct, class etc.):
+  // class A { } Object1, Object2;  <-- won't be matched
+  Finder->addMatcher(
----------------
aaron.ballman wrote:
> Why do we not want to match this?
If we decide, whether we transform 
```
class A { 
} Object1, Object2;
``` 
to
```
class A { 
} Object1, 
Object2;
``` 
or
```
class A { 
} 
Object1, 
Object2;
``` 
I might consider adding support for it. Moreover, this kind of definition is usually seen globally and I don't know how to handle globals yet. See http://lists.llvm.org/pipermail/cfe-dev/2015-November/046262.html


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:53
+  const LangOptions &LangOpts = getLangOpts();
+  const auto DeclGroup = DeclStatement->getDeclGroup();
+
----------------
aaron.ballman wrote:
> Please don't use `auto` as the type is not spelled out explicitly in the initialization. Same elsewhere, though it is fine with `diag()` and in for loop initializers.
Alright. What about:
```
const auto FirstVarIt = DeclStmt->getDeclGroup().begin();
const auto DL = SM.getDecomposedLoc(Loc);
```



================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:61
+      DeclStmtStart,
+      "declaration statement can be split up into single line declarations");
+
----------------
aaron.ballman wrote:
> This reads a bit awkwardly since it doesn't explain why the code is problematic. Perhaps: "multiple declarations should be split into individual declarations to improve readability" or something like that?
Yes. Sounds better.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:83
+      if (AnyTokenBetweenCommaAndVarName.front() == '#')
+        AnyTokenBetweenCommaAndVarName.insert(0, "\n");
+
----------------
aaron.ballman wrote:
> Will this (and the below `\n`) be converted into a CRLF automatically on platforms where that is the newline character sequence in the source?
Actually, I was thinking about this but I don't have a Windows etc. with a running llvm environment. Is it OK, if I leave a FIXME for now?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:101
+  QualType Type;
+  if (const auto *FirstVar = dyn_cast<const DeclaratorDecl>(*FirstVarIt)) {
+    Location = FirstVar->getLocation();
----------------
aaron.ballman wrote:
> You can drop the `const` from the `dyn_cast`, here and elsewhere.
Alright, but can you explain why?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:107
+    Type = FirstVar->getTypeSourceInfo()->getType();
+    while (Type->isLValueReferenceType()) {
+      Type = Type->getPointeeType();
----------------
aaron.ballman wrote:
> Why references and not pointers? Perhaps this needs a comment.
`// Typedefs on function pointers are covered into LValueReferenceType's` ?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:137
+    while (Type->isAnyPointerType() || Type->isArrayType())
+      Type = Type->getPointeeOrArrayElementType()->getCanonicalTypeInternal();
+  }
----------------
aaron.ballman wrote:
> Why are you calling the Internal version of this function?
Because I need its QualType.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:160
+
+static bool isWhitespaceExceptNL(unsigned char C) {
+  switch (C) {
----------------
aaron.ballman wrote:
> Instead of adding yet another custom check for whether something is whitespace, can you use `isWhitespace()` from CharInfo.h, and special case `\n`? Also, why `unsigned char` instead of `char`?
CharInfo, nice! `unsigned char` because there is no negative ASCII. Even isWhitespace has `unsigned char`.


================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:64
+    CheckFactories.registerCheck<OneNamePerDeclarationCheck>(
+        "readability-one-name-per-declaration");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
----------------
aaron.ballman wrote:
> Should this also be registered as an alias for cert-dcl04-c and CppCoreGuideline ES.10, or is there work left to be done for those checks?
I will include Options for these checks in the future, since they have special rules when to split a declaration. See https://reviews.llvm.org/D25024#inline-221483


https://reviews.llvm.org/D27621