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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
