kbobyrev added a comment.
A lot of good improvements and many test cases, thank you!
The comments are mostly nits.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast<unsigned int>(
----------------
JonasToth wrote:
> kbobyrev wrote:
> > How about `multiple declarations within a single statement hurts
> > readability`?
> s/hurts/reduces/? hurts sound a bit weird i think.
>
> Lebedev wanted the number of decls in the diagnostic, would you include it or
> rather now?
"decreases" is also fine. "hurts" is probably too strong, I agree.
Up to you. Personally, I don't see any value in having the diagnostic message
saying "hey, you have 2 declarations within one statement, that's really bad!"
or "hey, you have 5 declarations within one statement..." - in both cases the
point is that there are *multiple* declarations. I also don't think it would
make debugging easier because you also check the formatting, so you already
imply that the correct number of declarations was detected.
I'm interested to know what @lebedev.ri thinks.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:39
+ const Stmt *const InitStmt = Node.getInit();
+ if (InitStmt)
+ return InnerMatcher.matches(*InitStmt, Finder, Builder);
----------------
nit: `return InitStmt ? InnerMatcher... : false;`
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:65
+
+ for (int i = Indirections; i > 0; --i) {
+ Start = findPreviousAnyTokenKind(Start, SM, LangOpts, tok::star, tok::amp);
----------------
`i`-> `I`
or even better `while (--Indirections)` since it's not used anywhere afterwards.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:68
+ if (Start.isInvalid() || Start.isMacroID())
+ return SourceLocation();
+ }
----------------
Also, I don't think this should happen with the correct behavior.
`llvm::Expected<T>` looks like a better alternative for error handling here.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:78
+static int countIndirections(const Type *T, int Indirections = 0) {
+ assert(T && "Require non-null");
+ if (T->isPointerType() || T->isReferenceType())
----------------
nit: "Type has to be set for *countIndirections()*"? The wording is not
optimal, but currently the assertion message doesn't say much about the error.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:93
+static bool IsMemberPointer(const VarDecl *D) {
+ if (D->getType()->isMemberPointerType() ||
+ D->getType()->isMemberDataPointerType() ||
----------------
just `return (Condition0 || Condition1 || Condition2);`
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:102
+static Optional<std::vector<SourceRange>>
+DeclSlice(const DeclStmt *DS, const SourceManager &SM,
+ const LangOptions &LangOpts) {
----------------
nit: naming (lowercase + more descriptive name/documentation about the returned
value).
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:166
+ VarDecl *CurrentDecl = dyn_cast<VarDecl>(*It);
+ assert(CurrentDecl && "Expect only VarDecls here");
+
----------------
This assertion message doesn't really give much information, too. Before you
inspect the code closely (e.g. to understand what "here" refers to), it would
be hard for other developers to understand what the issue might just by looking
at the failed assertion message unless they are very familiar with the code.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:168
+
+ // FIXME Memberpointer are not transformed correctly right now, thats
+ // why they are treated as problematic here.
----------------
nit `FIXME:`. Also, ideally there would be an XFAIL test on that.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:173
+
+ SourceLocation DeclEnd;
+ if (CurrentDecl->hasInit())
----------------
nit: `const SourceLocation DeclEnd = findNextTerimator(CurrentDecl->hasInit() ?
CurrentDecl->getINit()->getEndLoc() : CurrentDecl->getEndLoc(), SM, LagOpts);`
(formatted with Clang-Format, of course, my snippet is messy :)
Up to you, though, I just think it would be slightly more readable.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:208
+static Optional<std::vector<StringRef>>
+SourceFromRanges(const std::vector<SourceRange> &Ranges,
+ const SourceManager &SM, const LangOptions &LangOpts) {
----------------
nit: `const std::vector<T> &` -> `llvm::ArrayRef<T>`
Also, probably `SourceFromRanges` -> `collectSourceText(Ranges, ...)`? This
would probably be easier to read (otherwise it's easier to confuse with
something like "get `SourceLocations` from `SourceRanges` when not looking at
the function body).
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:211
+ std::vector<StringRef> Snippets;
+ Snippets.reserve(Ranges.size());
+
----------------
nit: It would be probably easier to have `Snippets(Ranges.size()` and then
insert each `Snippet` to the correct position. That would require sacrificing
for-range loop and having a counter, but I think that it would make this piece
slightly better. Up to you, I don't have strong opinion about this part.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:214
+ for (const auto &Range : Ranges) {
+ CharSourceRange CR = Lexer::getAsCharRange(
+ CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM,
----------------
probably `auto CharRange = ...`? The type is more or explicitly typed in the
right-hand side expression and for seeing what `CR` actually is in the code
below it's easier to have more descriptive variable name rather than explicit
type in the declaration LHS (if I understand the trade-off correctly here).
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:218
+
+ if (CR.isInvalid()) {
+#if PRINT_DEBUG
----------------
(I have another comment about `llvm::Optional` vs `llvm::Expected` below, more
information there)
Many of these checks actually print something to `std::cerr` and they look like
actual errors (e.g. I don't think `CharSourceRanges` should be invalid in some
scenario), so I think these should be treated like errors.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:244
+static std::vector<std::string>
+CreateIsolatedDecls(const std::vector<StringRef> &Snippets) {
+ // The first section is the type snippet, which does not make a decl itself.
----------------
Same here (`llvm::ArrayRef`)
Also, functions should start with lowercase (i.e. -> `createIsolatedDecls`).
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:249
+
+ for (std::size_t i = 1, End = Snippets.size(); i < End; ++i)
+ Decls[i - 1] = Twine(Snippets[0]).concat(Snippets[i].ltrim()).str();
----------------
Same here, please use `I` instead of `i`. Also, `Snippets` is just a vector,
therefore having `I < Snippets.size()` wouldn't be expensive (which is usually
the reason behind storing `End` iterator in the cycle).
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:255
+
+static std::string Concat(const std::vector<std::string> &Decls,
+ StringRef Indent) {
----------------
Maybe just inline it? This is only one line and it is only used once.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:268
+
+ Optional<std::vector<SourceRange>> PotentialSlices =
+ DeclSlice(WholeDecl, *Result.SourceManager, getLangOpts());
----------------
nit: `auto` here? Since you're not actually using the variable a lot and just
pass to other functions, it might be easier to read if the type was deduced
autmatically.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:273
+
+ const std::vector<SourceRange> &Slices = *PotentialSlices;
+ Optional<std::vector<StringRef>> PotentialSnippets =
----------------
nit: `llvm::ArrayRef` or just inline it in the function call next line (is
probably better).
Same below.
================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:278
+ if (!PotentialSnippets)
+ return;
+
----------------
Both for `PotentialSlices` and `PotentialSnippets`: is there any case where any
can not be determined and this is not an error? They both are
`llvm::Optional<T>`s and the `check(Result)` just returns if any of them are
not set, is there any case when any of the variables is actually not set, but
this is the correct behavior? If not, IMO it would be better to use
`llvm::Expected`: then, if the check fails, either print `.takeError()` message
or propagate it "upstairs" (although I'm not really sure what would be better
here).
================
Comment at: test/clang-tidy/readability-isolate-decl.cpp:142
+ int member1, member2;
+ // TODO Transform FieldDecl's as well
+};
----------------
nit `FIXME` (and also XFAIL test if you have enough time, it's fine to leave
`FIXME`s in general, though.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
