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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

Sorry, I didn't get time to review the patch properly, these are few stylistic comments. Hopefully, I'll be able to give more feedback when I get more time.



================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:21
+
+#define PRINT_DEBUG 1
+
----------------
Do you plan to submit the patch with debugging messages or are you just using these for better local debugging experience?

If you plan to upload the patch with the messages, please use `LLVM_DEBUG` (see `readability/IdentifierNamingCheck.cpp` for reference) and `llvm::dbgs()` ([[ https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden | `<iostream>` traits shouldn't be used ]] and should be replaced with their LLVM counterparts: `llvm::outs()`, `llvm::errs()`, etc). Also, I believe the messages should be more informative if you think debug info is really important here.


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:63
+
+    if (!CurrentToken.hasValue())
+      return SourceLocation();
----------------
nit: `if (!CurrentToken)`


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:323
+
+static std::string &TrimRight(std::string &str) {
+  auto it1 = std::find_if(str.rbegin(), str.rend(), [](char ch) {
----------------
`llvm::StringRef::rtrim()`?

Also, naming `str` should be at least `Str`, `it1` -> `It`, `ch` -> `Ch` (or better `Symbol`) ...


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:331
+
+static std::string Concat(const std::vector<std::string> &Decls,
+                          StringRef Indent) {
----------------
Use `llvm::join` with `";\n" + Indent` as the `Separator`?


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:333
+                          StringRef Indent) {
+  std::string R;
+  for (const StringRef &D : Decls)
----------------
`Range`?


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+  auto Diag =
+      diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+      << static_cast<unsigned int>(
----------------
How about `multiple declarations within a single statement hurts readability`?


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:51
+
+    return TypeAndName + " = " + Initializer + ";";
+  }
----------------
JonasToth wrote:
> kbobyrev wrote:
> > JonasToth wrote:
> > > kbobyrev wrote:
> > > > This seems to replace `int x = 5, y = 42;` with `int x = 5;int y = 42`. I don't think that it becomes cleaner (in fact, without spaces in between it looks cryptic). Consider formatting it or simply applying newlines (if there were no newlines inbetween before).
> > > I do not plan to do a lot of formatting here (maybe space or newline), because that clang-format area.
> > While Clang-Tidy can apply Clang-Format on top of the Fix-Its, it will still look weird in the Fix-Its previews. While supporting proper formatting, in general, might be hard, it totally makes sense to do some basic formatting so that editor integration warnings would look better, for example.
> The current version adds a new line for each decl and keeps the indendation (as the other check does).
> 
> Because it does the slicing on commas the manual/custom formatting of the original code will stay. That might result in weird looking output for exotic variable declarations. I would like to ignore these cases, what do you think @kbobyrev ?
Yes, sure, it's hard (and probably impossible) to support the generic case. This approach sounds good to me, thanks!


================
Comment at: clang-tidy/readability/IsolateDeclCheck.h:1
+//===--- IsolateDeclCheck.h - clang-tidy-------------------------*- C++ -*-===//
+//
----------------
JonasToth wrote:
> kbobyrev wrote:
> > nit: space between clang-tidy (also, in .cpp file)
> That comes from the template `add_new_check.py`, do you want me to fix it there?
Ah, okay, I was thinking whether it was a template issue (since I thought it's unlikely that one would edit the header template). Yes, it looks like a typo in the `clang-tidy` check adding tool implementation. It seems to be fixed in rL342601.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949