njames93 marked 4 inline comments as done.
njames93 added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:51
ClangTidyContext *Context)
- : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions),
+ : NamePrefix((CheckName + ".").str()), CheckOptions(CheckOptions),
Context(Context) {}
----------------
LegalizeAdulthood wrote:
> Why is `StringRef::operator+` considered "better" than
> `std::string::operator+`?
Because 2 strings are created in the second instance, whereas only one is
created in the first.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:56
ClangTidyCheck::OptionsView::get(StringRef LocalName) const {
- const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
+ const auto &Iter = CheckOptions.find((NamePrefix + LocalName).str());
if (Iter != CheckOptions.end())
----------------
LegalizeAdulthood wrote:
> `find` takes a `StringRef` so why convert to `std::string` here?
Because a concatenation of StringRef results in a Twine, which cannot be passed
to find.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:123
StringRef Value) const {
- Options[NamePrefix + LocalName.str()] = Value;
+ Options[(NamePrefix + LocalName).str()] = Value;
}
----------------
LegalizeAdulthood wrote:
> `operator[]` takes a `StringRef` so why convert to `std::string` here?
Again can't use a Twine for operator[].
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:158
/// ``None``.
- llvm::Optional<std::string> get(StringRef LocalName) const;
+ llvm::Optional<StringRef> get(StringRef LocalName) const;
----------------
LegalizeAdulthood wrote:
> I think I would feel safer if the changes to the infrastructure were pushed
> separately from the changes to the checks, with some "bake time" in between
> so we have more confidence in the changes.
>
> While debugging my own checks, I've seen that there are surprises with the
> lifetimes of `StringRef`.
These changes on their own don't make any sense without the corresponding
changes to the checks.
Also the lifetime of all the checks options is tied to the ClangTidyOptions
which outlives the checks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124341/new/
https://reviews.llvm.org/D124341
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
