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

Richard via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 27 10:28:53 PDT 2022

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

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list