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