[PATCH] D124341: [clang-tidy][NFC] Replace many instances of std::string where a StringRef would suffice.

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood 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) {}
----------------
Why is `StringRef::operator+` considered "better" than `std::string::operator+`?


================
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())
----------------
`find` takes a `StringRef` so why convert to `std::string` here?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:123
                                         StringRef Value) const {
-  Options[NamePrefix + LocalName.str()] = Value;
+  Options[(NamePrefix + LocalName).str()] = Value;
 }
----------------
`operator[]` takes a `StringRef` so why convert to `std::string` here?


================
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;
 
----------------
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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124341/new/

https://reviews.llvm.org/D124341