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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:21
+
+#define PRINT_DEBUG 1
+
----------------
kbobyrev wrote:
> 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.
No, they will be removed, just for the current prototyping. using `llvm::outs` and `llvm::errs` would have been better though ;)


================
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) {
----------------
kbobyrev wrote:
> `llvm::StringRef::rtrim()`?
> 
> Also, naming `str` should be at least `Str`, `it1` -> `It`, `ch` -> `Ch` (or better `Symbol`) ...
`rtrim()` is a better fit, but introduces another copy. Do you think I should rater use `llvm::SmallString`?


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:331
+
+static std::string Concat(const std::vector<std::string> &Decls,
+                          StringRef Indent) {
----------------
kbobyrev wrote:
> Use `llvm::join` with `";\n" + Indent` as the `Separator`?
Yup, didn't know that one yet, thanks :)


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343
+  auto Diag =
+      diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+      << static_cast<unsigned int>(
----------------
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?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949