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