[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

[Kirill Bobyrev via Phabricator via cfe-commits]

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