[PATCH] D88314: Added llvm-string-referencing check

[Aaron Ballman via Phabricator via cfe-commits]

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