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

[Nathan James via Phabricator via cfe-commits]

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