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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:22
+
+const internal::VariadicDynCastAllOfMatcher<Decl, TagDecl> tagDecl;
+
----------------
We may want to consider adding this to ASTMatchers.h at some point (can be done in a future patch).


================
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,
----------------
This should accept by `const std::string &`.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:32
+  // Matches all non-single declaration within a compound statement {...}.
+  // Unless, the variable declaration is a object definition directly after
+  // a tag declaration (e.g. struct, class etc.):
----------------
is a object -> is an object


================
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(
----------------
Why do we not want to match this?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:44
+  const auto *DeclStatement = Result.Nodes.getNodeAs<DeclStmt>("declstmt");
+  if (DeclStatement == nullptr)
+    return;
----------------
No need to compare against nullptr explicitly (I think we have a clang-tidy check that will warn on this, in fact).


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:53
+  const LangOptions &LangOpts = getLangOpts();
+  const auto DeclGroup = DeclStatement->getDeclGroup();
+
----------------
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.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:61
+      DeclStmtStart,
+      "declaration statement can be split up into single line declarations");
+
----------------
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?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:67
+  for (auto It = DeclGroup.begin() + 1; It != DeclGroup.end(); ++It) {
+
+    const auto NameLocation = dyn_cast<const NamedDecl>(*It)->getLocation();
----------------
No need for the newline.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:81
+
+      // Check for pre-processor directive and add appropriate newline
+      if (AnyTokenBetweenCommaAndVarName.front() == '#')
----------------
Missing a full stop at the end of the sentence. Also, no hyphen in preprocessor, and it should be "add an appropriate".


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:83
+      if (AnyTokenBetweenCommaAndVarName.front() == '#')
+        AnyTokenBetweenCommaAndVarName.insert(0, "\n");
+
----------------
Will this (and the below `\n`) be converted into a CRLF automatically on platforms where that is the newline character sequence in the source?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:87
+           << FixItHint::CreateReplacement(AfterCommaToVarNameRange,
+                                           "\n" + CurrentIndent +
+                                               UserWrittenType + " " +
----------------
This should probably use a `Twine` to avoid a bunch of copies.


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


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:107
+    Type = FirstVar->getTypeSourceInfo()->getType();
+    while (Type->isLValueReferenceType()) {
+      Type = Type->getPointeeType();
----------------
Why references and not pointers? Perhaps this needs a comment.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:131
+      Type->isFunctionPointerType() || Type->isFunctionProtoType()) {
+    Pos = UserWrittenType.find_first_of("&*(");
+    if (Pos != std::string::npos) { // might be hidden behind typedef etc.
----------------
You may want to include tests for pathological code, like:
```
int *(i), (*j), (((k)));
```


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:132
+    Pos = UserWrittenType.find_first_of("&*(");
+    if (Pos != std::string::npos) { // might be hidden behind typedef etc.
+      UserWrittenType.erase(Pos);
----------------
`m` should be capitalized, and you can elide the braces.


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


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:148
+      Pos != std::string::npos) {
+    // user might have inserted additional whitespaces:
+    // int   S  ::
----------------
Capitalize "u".


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:160
+
+static bool isWhitespaceExceptNL(unsigned char C) {
+  switch (C) {
----------------
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`?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:200
+  StringRef IndentSpace;
+  {
+    size_t i = LineOffs;
----------------
Why the extra compound statement?


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:201
+  {
+    size_t i = LineOffs;
+    while (isWhitespaceExceptNL(MB[i])) {
----------------
Should be `I` instead of `i`.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:246
+
+static std::string getCurrentLineIndent(SourceLocation Loc,
+                                        const SourceManager &SM) {
----------------
firolino wrote:
> malcolm.parsons wrote:
> > Should checks care about formatting, or leave to clang-format?
> At least, they should not change the formatting and try to preserve it. The user shouldn't be forced to run clang-format multiple times. Moreover, not everyone is using clang-format etc.
> 
> Having:
> ```
> const int myvalue( 42 ), value ( 4 );
> 
> {
>     int a, b, c;
> }
> ```
> and getting:
> ```
> const int myvalue(42);
> const int value(4);
> 
> {
>     int a;
> int b;
> int c;
> }
> ```
> seems very unsatisfying to me. This would force a non-formatting-tool user to start using one.
I think what @malcolm.parsons was getting at is that we have talked about automatically running clang-format over changes made from the FixIt engine instead of trying to add custom logic to all of the places we use FixIts.


================
Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:64
+    CheckFactories.registerCheck<OneNamePerDeclarationCheck>(
+        "readability-one-name-per-declaration");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
----------------
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?


================
Comment at: clang-tidy/utils/LexerUtils.h:15
 #include "clang/Lex/Lexer.h"
+#include <vector>
 
----------------
Please remove this include.


================
Comment at: docs/ReleaseNotes.rst:142
+
+  Finds declarations declaring more that one name.
+
----------------
that -> than


================
Comment at: docs/clang-tidy/checks/readability-one-name-per-declaration.rst:6
+
+This check can be used to find declarations, which declare more than one name. 
+It helps improving readability and prevents potential bugs caused by inattention
----------------
Remove the comma. which -> that.


================
Comment at: docs/clang-tidy/checks/readability-one-name-per-declaration.rst:7
+This check can be used to find declarations, which declare more than one name. 
+It helps improving readability and prevents potential bugs caused by inattention
+and C/C++ syntax specifics.
----------------
improving -> improve


================
Comment at: test/clang-tidy/readability-one-name-per-declaration-complex.cpp:244
+}
+
----------------
Can you add a test with:
```
template <typename A, typename B> // Should not be touched
void f(A, B);
```


https://reviews.llvm.org/D27621