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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88314#2322870 <https://reviews.llvm.org/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)?



================
Comment at: clang-tools-extra/clang-tidy/add_new_check.py:139
   const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("x");
+<<<<<<< HEAD
+  if ((!MatchedDecl->getIdentifier()) || MatchedDecl->getName().startswith("awesome_"))
----------------
Looks like there are some accidental merge markers in the patch.


================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:204
   void AwesomeFunctionNamesCheck::check(const MatchFinder::MatchResult &Result) {
+<<<<<<< HEAD
+    const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("x");    
----------------
More merge conflict markers.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/llvm-string-referencing.cpp:25
+void f1(const string &P);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: parameter of type 'const std::string &' could potentially be replaced with 'llvm::StringRef' [llvm-string-referencing]
+// CHECK-FIXES: void f1(llvm::StringRef P);{{$}}
----------------
You can drop the name of the check from the `CHECK-MESSAGES` line (we usually only check the full diagnostic once).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88314/new/

https://reviews.llvm.org/D88314