aaron.ballman added a comment.
Thank you for working on this check! Have you run the check over LLVM to see
how much it reports? One of the concerns I have with this is that it's not
always appropriate to replace a `const std::string&` with an `llvm::StringRef`
and so I'm wondering what the "false positive" rate is for the check. For
instance, there are `std::string` member functions that don't exist on
`StringRef` so the fix-it will introduce compile errors, or switching the
parameter type will cause additional unnecessary conversions.
================
Comment at: clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp:23
+void StringReferencingCheck::registerMatchers(MatchFinder *Finder) {
+ // create a matcher looking for const std::string& parameters
+ auto Matcher =
----------------
Comments should be grammatical with capital letters and punctuation (here and
elsewhere in the patch).
Also, I don't think we should register this check for the handful of C files.
================
Comment at: clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp:25
+ auto Matcher =
+ parmVarDecl(hasType(references(namedDecl(
+ hasName("string"), isInStdNamespace()))), // std::string&
----------------
Do we want to ignore parameters of a virtual function under the assumption that
changing the parameter type may be dangerous?
================
Comment at: clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp:26
+ parmVarDecl(hasType(references(namedDecl(
+ hasName("string"), isInStdNamespace()))), // std::string&
+ hasType(references(qualType(isConstQualified()))), // const
----------------
Instead of checking `isInStdNamespace()`, you can use
`hasName("::std::string")`.
================
Comment at: clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp:35
+void StringReferencingCheck::registerPPCallbacks(
+ const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP)
{
+ Inserter.registerPreprocessor(PP);
----------------
You can elide the identifiers `SM` and `ModuleExpanderPP` to shorten the
declaration (since the names aren't used anyway).
================
Comment at: clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp:50
+ // Generate diagnostics and fix
+ diag(ParamBegin, "Use of const std::string & is discouraged in the LLVM "
+ "Programmer's Manual");
----------------
clang-tidy diagnostics are not supposed to be grammatical, so it should not
start with a capital letter and shouldn't use terminating punctuation, etc. How
about something like: `parameter of type 'const std::string &' could
potentially be replaced with 'llvm::StringRef'`?
================
Comment at: clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp:52
+ "Programmer's Manual");
+ diag(ParamBegin, "replace parameter %0 type with llvm::StringRef",
+ DiagnosticIDs::Note)
----------------
I don't think the note helps a whole lot, you could attach the fix-it to the
warning itself. However, we usually only add a fix-it when we can be sure that
the change will continue to compile and I don't know that's the case here.
================
Comment at: clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.h:26
+class StringReferencingCheck : public ClangTidyCheck {
+ const llvm::StringRef BoundNodeId = "string_reference";
+
----------------
I think this can be hidden in the implementation file rather than exposed in
the header. Also, can you pick a more descriptive name, like `ParamID` or
something?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/llvm-string-referencing.rst:6
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
----------------
Can you add some documentation?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88314/new/
https://reviews.llvm.org/D88314
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
