Added llvm-string-referencing check

[bogser01]

Summary

Clang-tidy pass detecting the use of const std::string& references.

Use of llvm::StringRef is recommended in the LLVM Programmer's Manual instead:
https://llvm.org/docs/ProgrammersManual.html#the-stringref-class

[bogser01]

Fixed failing unit test

[bogser01]

Fixed unit test 2

[bogser01]

Removed residual conflict markers

[bogser01]

Remove conflict markers 2

[bogser01]

@alexfh does this look alright?

[aaron.ballman]

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.

clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp

23

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.

25

Do we want to ignore parameters of a virtual function under the assumption that changing the parameter type may be dangerous?

26

Instead of checking isInStdNamespace(), you can use hasName("::std::string").

35

You can elide the identifiers SM and ModuleExpanderPP to shorten the declaration (since the names aren't used anyway).

50

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'?

52

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.

clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.h

26

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?

clang-tools-extra/docs/clang-tidy/checks/llvm-string-referencing.rst

6

Can you add some documentation?

[bogser01]

Added documentation & Nit fixes

[aaron.ballman]

It looks like you generated a diff from a previous diff instead of trunk -- can you regenerate the diff against trunk so that reviewers can see the full content of the changes?

[bogser01]

@aaron.ballman Thank you for picking up this review! Running the check over the entire LLVM causes ~74K warnings across 430 files. As to the false positive rate it's tricky to measure. Based on previous analysis on flang codebase, I would say roughly 50% of the fixes would cause compiler errors when applied.

[bogser01]

Rebase diff

[aaron.ballman]

In D88314#2322870, @bogser01 wrote:

@aaron.ballman Thank you for picking up this review! Running the check over the entire LLVM causes ~74K warnings across 430 files. As to the false positive rate it's tricky to measure. Based on previous analysis on flang codebase, I would say roughly 50% of the fixes would cause compiler errors when applied.

Woof, that's a pretty high false-positive rate for the check -- I don't think we should issue a fix-it for this case because anyone who accidentally applies fixits automatically will be in for a fair amount of pain.

I'm a bit worried that the number of diagnostics this produces will basically mean the check has to be turned off for the only project that would use it. What is the expected use case for the check? For instance, are you expecting to craft a .clang-tidy file to disable this check on source files in the repo that are known to have a lot of diagnostics (so that the check mostly only fires for known-good files and new files)?

clang-tools-extra/clang-tidy/add_new_check.py

139 ↗

(On Diff #297355)

Looks like there are some accidental merge markers in the patch.

clang-tools-extra/docs/clang-tidy/Contributing.rst

204 ↗

(On Diff #297355)

More merge conflict markers.

clang-tools-extra/test/clang-tidy/checkers/llvm-string-referencing.cpp

26

You can drop the name of the check from the CHECK-MESSAGES line (we usually only check the full diagnostic once).

clang-tools-extra/clang-tidy/llvm/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyLLVMModule
   HeaderGuardCheck.cpp
   IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
   PreferIsaOrDynCastInConditionalsCheck.cpp
   PreferRegisterOverUnsignedCheck.cpp
+  StringReferencingCheck.cpp
   TwineLocalCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
 
   DEPENDS

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "../readability/ElseAfterReturnCheck.h"
 #include "../readability/NamespaceCommentCheck.h"
 #include "../readability/QualifiedAutoCheck.h"
 #include "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
 #include "PreferIsaOrDynCastInConditionalsCheck.h"
 #include "PreferRegisterOverUnsignedCheck.h"
+#include "StringReferencingCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm_check {
 
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<readability::ElseAfterReturnCheck>(
         "llvm-else-after-return");
     CheckFactories.registerCheck<LLVMHeaderGuardCheck>("llvm-header-guard");
     CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order");
     CheckFactories.registerCheck<readability::NamespaceCommentCheck>(
         "llvm-namespace-comment");
     CheckFactories.registerCheck<PreferIsaOrDynCastInConditionalsCheck>(
         "llvm-prefer-isa-or-dyn-cast-in-conditionals");
     CheckFactories.registerCheck<PreferRegisterOverUnsignedCheck>(
         "llvm-prefer-register-over-unsigned");
     CheckFactories.registerCheck<readability::QualifiedAutoCheck>(
         "llvm-qualified-auto");
+    CheckFactories.registerCheck<StringReferencingCheck>(
+        "llvm-string-referencing");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     Options.CheckOptions["llvm-qualified-auto.AddConstToQualified"] = "0";
     Options.CheckOptions["llvm-else-after-return.WarnOnUnfixable"] = "0";
     Options.CheckOptions["llvm-else-after-return.WarnOnConditionVariables"] =

clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.h

+//===--- StringReferencingCheck.h - clang-tidy ------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_STRINGREFERENCINGCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_STRINGREFERENCINGCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "../utils/IncludeInserter.h"
+#include "clang/Frontend/CompilerInstance.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm_check {
+
+/// LThe LLVM Programmer's Manual recommends that llvm::StringRef should be
+/// used for function parameters instead of references to const std::string:
+/// https://llvm.org/docs/ProgrammersManual.html#the-stringref-class
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-string-referencing.html
+class StringReferencingCheck : public ClangTidyCheck {
+  const llvm::StringRef BoundNodeId = "string_reference";
+
+public:
+  StringReferencingCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        Inserter(Options.getLocalOrGlobal("IncludeStyle",
+                                          utils::IncludeSorter::IS_LLVM)) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  utils::IncludeInserter Inserter;
+};
+
+} // namespace llvm_check
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_STRINGREFERENCINGCHECK_H

clang-tools-extra/clang-tidy/llvm/StringReferencingCheck.cpp

+//===--- StringReferencingCheck.cpp - clang-tidy --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "StringReferencingCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace llvm_check {
+
+void StringReferencingCheck::registerMatchers(MatchFinder *Finder) {
+  // create a matcher looking for const std::string& parameters
+  auto Matcher =
+      parmVarDecl(hasType(references(namedDecl(
+                      hasName("string"), isInStdNamespace()))), // std::string&
+                  hasType(references(qualType(isConstQualified()))), // const
+                  hasType(lValueReferenceType()) // don't match std::string&&
+      );
+  // Add matcher to the finder and bound matched nodes
+  Finder->addMatcher(Matcher.bind(BoundNodeId), this);
+}
+
+void StringReferencingCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  Inserter.registerPreprocessor(PP);
+}
+
+void StringReferencingCheck::check(const MatchFinder::MatchResult &Result) {
+  // Retrieve parameters found by matcher
+  const auto *ParamDecl = Result.Nodes.getNodeAs<ParmVarDecl>(BoundNodeId);
+  // Extract parameter type location in source
+  clang::SourceRange TypeRange;
+  auto ParamBegin = ParamDecl->getBeginLoc();
+  if (!ParamBegin.isValid())
+    return;
+  TypeRange.setBegin(ParamBegin);
+  TypeRange.setEnd(ParamDecl->getTypeSpecEndLoc());
+  // Generate diagnostics and fix
+  diag(ParamBegin, "Use of const std::string & is discouraged in the LLVM "
+                   "Programmer's Manual");
+  diag(ParamBegin, "replace parameter %0 type with llvm::StringRef",
+       DiagnosticIDs::Note)
+      << ParamDecl
+      << FixItHint::CreateReplacement(TypeRange, "llvm::StringRef ")
+      << Inserter.createMainFileIncludeInsertion(
+             "llvm/include/llvm/ADT/StringRef.h",
+             /*IsAngled=*/false);
+}
+
+} // namespace llvm_check
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `hicpp-no-assembler <hicpp-no-assembler.html>`_,
    `hicpp-signed-bitwise <hicpp-signed-bitwise.html>`_,
    `linuxkernel-must-use-errs <linuxkernel-must-use-errs.html>`_,
    `llvm-header-guard <llvm-header-guard.html>`_,
    `llvm-include-order <llvm-include-order.html>`_, "Yes"
    `llvm-namespace-comment <llvm-namespace-comment.html>`_,
    `llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm-prefer-isa-or-dyn-cast-in-conditionals.html>`_, "Yes"
    `llvm-prefer-register-over-unsigned <llvm-prefer-register-over-unsigned.html>`_, "Yes"
+   `llvm-string-referencing <llvm-string-referencing.html>`_, "Yes"
    `llvm-twine-local <llvm-twine-local.html>`_, "Yes"
    `llvmlibc-callee-namespace <llvmlibc-callee-namespace.html>`_,
    `llvmlibc-implementation-in-namespace <llvmlibc-implementation-in-namespace.html>`_,
    `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
    `misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
    `misc-misplaced-const <misc-misplaced-const.html>`_,
    `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `misc-no-recursion <misc-no-recursion.html>`_,

clang-tools-extra/docs/clang-tidy/checks/llvm-string-referencing.rst

+.. title:: clang-tidy - llvm-string-referencing
+
+llvm-string-referencing
+=======================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

clang-tools-extra/test/clang-tidy/checkers/llvm-string-referencing.cpp

+// RUN: %check_clang_tidy %s llvm-string-referencing %t
+
+namespace std {
+class string {};
+class u18_string_t;
+
+} // namespace std
+
+namespace llvm {
+class StringRef;
+} // namespace llvm
+
+class String;
+
+namespace A {
+using namespace std;
+void f(const string &P);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+// CHECK-FIXES: void f(llvm::StringRef P);{{$}}
+} // namespace A
+
+namespace B {
+using std::string;
+void f1(const string &P);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+// CHECK-FIXES: void f1(llvm::StringRef P);{{$}}
+} // namespace B
+
+void f2(std::string, int, const std::string &);
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+// CHECK-FIXES: void f2(std::string, int, llvm::StringRef );{{$}}
+void f2(std::string P, int x, const std::string &P2) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+  // CHECK-FIXES: void f2(std::string P, int x, llvm::StringRef P2) {{{$}}
+  return;
+}
+
+void f3(const std::string &P1, const std::string &P2);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+// CHECK-MESSAGES: :[[@LINE-2]]:32: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+// CHECK-FIXES: void f3(llvm::StringRef P1, llvm::StringRef P2);{{$}}
+
+struct St {
+  void operator=(const std::string &Val) const {
+    // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+    // CHECK-FIXES:     void operator=(llvm::StringRef Val) const {{{$}}
+    return;
+  }
+};
+
+void f7(const std::string &);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Use of const std::string & is discouraged in the LLVM Programmer's Manual [llvm-string-referencing]
+// CHECK-FIXES: void f7(llvm::StringRef );{{$}}
+
+// Functions below this line should not trigger the check
+void f1(std::string &P);
+
+void f4(std::string *P);
+
+void f5(String &P);
+
+void f6(llvm::StringRef P);
+
+void f9(std::u18_string_t &P);
+
+void f10(const std::string &&P);