[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;
+
----------------
firolino wrote:
> 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?
You could put in a FIXME, or just create the ASTMatchers patch and mark this patch as dependent on that one.


================
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(
----------------
firolino wrote:
> 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
I think this should be handled. It can be handled in either of the forms you show, or by saying:
```
A Object1;
A Object2;
```
If all of these turn out to be a problem, we can still diagnose without providing a fixit.

As for globals in general, they can be handled in a separate patch once we figure out the declaration grouping.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:53
+  const LangOptions &LangOpts = getLangOpts();
+  const auto DeclGroup = DeclStatement->getDeclGroup();
+
----------------
firolino wrote:
> 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);
> ```
> 
Iterators are fine (because they usually involve a lot of template boilerplate for containers and their interfaces are regular and common enough), but the call to `getDecomposedLoc()` should not use `auto` -- the reader requires too much internal knowledge to understand what the return type is.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:83
+      if (AnyTokenBetweenCommaAndVarName.front() == '#')
+        AnyTokenBetweenCommaAndVarName.insert(0, "\n");
+
----------------
firolino wrote:
> 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?
I think it's fine with or without the fixme (it's certainly not the first instance of `\n` in the compiler).


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:101
+  QualType Type;
+  if (const auto *FirstVar = dyn_cast<const DeclaratorDecl>(*FirstVarIt)) {
+    Location = FirstVar->getLocation();
----------------
firolino wrote:
> aaron.ballman wrote:
> > You can drop the `const` from the `dyn_cast`, here and elsewhere.
> Alright, but can you explain why?
The `const` on the declaration is sufficient to ensure const correctness. It's morally equivalent to:
```
void f(int i) {
  const int ci = i; // No need to const_cast this either.
}
```


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:107
+    Type = FirstVar->getTypeSourceInfo()->getType();
+    while (Type->isLValueReferenceType()) {
+      Type = Type->getPointeeType();
----------------
firolino wrote:
> aaron.ballman wrote:
> > Why references and not pointers? Perhaps this needs a comment.
> `// Typedefs on function pointers are covered into LValueReferenceType's` ?
So should this be specific to function pointer types? If so, I don't see that predicated anywhere.


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:137
+    while (Type->isAnyPointerType() || Type->isArrayType())
+      Type = Type->getPointeeOrArrayElementType()->getCanonicalTypeInternal();
+  }
----------------
firolino wrote:
> aaron.ballman wrote:
> > Why are you calling the Internal version of this function?
> Because I need its QualType.
You should not call functions that have Internal in the name (generally speaking). Those are generally considered to be warts we want to remove once we get rid of the last external caller.

All of your uses of `Type` are as a `clang::Type *`, so are you sure you need the `QualType` at all? (Btw, you should consider a better name than `Type`.)


================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:160
+
+static bool isWhitespaceExceptNL(unsigned char C) {
+  switch (C) {
----------------
firolino wrote:
> 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`.
Hmm, I'm used to seeing characters using `char` and arithmetic types using `unsigned char`, but I suppose I've also seen UTF-8 code points represented as `unsigned char`, so good enough for me. :-)


https://reviews.llvm.org/D27621