[PATCH] D91015: [clang-tidy] Extend bugprone-string-constructor-check to std::string_view.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:29-33
+static ast_matchers::internal::Matcher<NamedDecl>
+hasAnyNameStdString(std::vector<std::string> Names) {
+  return ast_matchers::internal::Matcher<NamedDecl>(
+      new ast_matchers::internal::HasNameMatcher(std::move(Names)));
+}
----------------
`hasAnyName` is a `VariadicMatcher`, so it supports passing a list directly (as a single argument), see ASTMatchersInternal.h, line 103.

Given that, I'm pretty sure this function is unnecessary; you can just call `hasAnyName` directly below instead of this function.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:112
       cxxConstructExpr(
-          hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+          hasDeclaration(cxxMethodDecl(hasStringCtorName)),
           hasArgument(0, hasType(CharPtrType)),
----------------
Following up on Aaron's comment above, I think you want something like this: `hasDeclaration(cxxConstructorDecl(ofClass(cxxRecordDecl(hasAnyName(removeNamespaces(StringNames))))`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:136
+                            anyOf(
+                                // do not match std::string(ptr, int)
+                                // match std::string(ptr, alloc)
----------------
nit: I'd put this comment line before the `anyOf` since it describes the whole thing, while the following comments describe each branch, respectively.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91015