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

[ckennelly]

Summary

This allows for matching the constructors std::string has in common with
std::string_view.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp

58

You should fix the lint warning.

59

It took me a while to realize that this functionality is really about trying to get to the name of the constructor for a given type -- perhaps this should be a single function called getCtorNameFromType() or something?

Actually, given that this is a narrowing matcher from a CXXConstructExpr, can't you look at CXXConstructExpr::getConstructor() to get the CXXConstructorDecl and check the name from there? That seems cleaner than trying to identify the constructor name through string matching.

[ymandel]

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp

29–33

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.

103–104

Following up on Aaron's comment above, I think you want something like this: hasDeclaration(cxxConstructorDecl(ofClass(cxxRecordDecl(hasAnyName(removeNamespaces(StringNames))))

128

nit: I'd put this comment line before the anyOf since it describes the whole thing, while the following comments describe each branch, respectively.

[ckennelly]

Applied review feedback

[ckennelly]

Applied review comments

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp

59

I applied ymandel's suggestion, which I think simplified things a bit.

[ymandel]

LGTM. I'll leave it to Aaron, though, to accept.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.h

public:
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   const bool WarnOnLargeLength;
   const unsigned int LargeLengthThreshold;
+  std::vector<std::string> StringNames;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRING_CONSTRUCTOR_H

clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp

 //===--- StringConstructorCheck.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 "StringConstructorCheck.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 namespace {
 AST_MATCHER_P(IntegerLiteral, isBiggerThan, unsigned, N) {
   return Node.getValue().getZExtValue() > N;
 }
+
+const char DefaultStringNames[] =
+    "::std::basic_string;::std::basic_string_view";
+
+static std::vector<StringRef>
+removeNamespaces(const std::vector<std::string> &Names) {
+  std::vector<StringRef> Result;
+  Result.reserve(Names.size());
+  for (StringRef Name : Names) {
+    std::string::size_type ColonPos = Name.rfind(':');
+    Result.push_back(
+        Name.substr(ColonPos == std::string::npos ? 0 : ColonPos + 1));
+  }
+  return Result;
+}
+
 } // namespace
 
 StringConstructorCheck::StringConstructorCheck(StringRef Name,
                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       WarnOnLargeLength(Options.get("WarnOnLargeLength", true)),
-      LargeLengthThreshold(Options.get("LargeLengthThreshold", 0x800000)) {}
+      LargeLengthThreshold(Options.get("LargeLengthThreshold", 0x800000)),
+      StringNames(utils::options::parseStringList(
+          Options.get("StringNames", DefaultStringNames))) {}
 
 void StringConstructorCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnLargeLength", WarnOnLargeLength);
   Options.store(Opts, "LargeLengthThreshold", LargeLengthThreshold);
+  Options.store(Opts, "StringNames", DefaultStringNames);
 }
 
 void StringConstructorCheck::registerMatchers(MatchFinder *Finder) {
   const auto ZeroExpr = expr(ignoringParenImpCasts(integerLiteral(equals(0))));
   const auto CharExpr = expr(ignoringParenImpCasts(characterLiteral()));
   const auto NegativeExpr = expr(ignoringParenImpCasts(
       unaryOperator(hasOperatorName("-"),
                     hasUnaryOperand(integerLiteral(unless(equals(0)))))));
   const auto LargeLengthExpr = expr(ignoringParenImpCasts(
       integerLiteral(isBiggerThan(LargeLengthThreshold))));
   const auto CharPtrType = type(anyOf(pointerType(), arrayType()));
 
   // Match a string-literal; even through a declaration with initializer.
Finder->addMatcher(
               hasArgument(0, LargeLengthExpr.bind("large-length"))))
           .bind("constructor"),
       this);
 
   // Check the literal string constructor with char pointer and length
   // parameters. [i.e. string (const char* s, size_t n);]
   Finder->addMatcher(
       cxxConstructExpr(
-          hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+          hasDeclaration(cxxConstructorDecl(ofClass(
+              cxxRecordDecl(hasAnyName(removeNamespaces(StringNames)))))),
           hasArgument(0, hasType(CharPtrType)),
           hasArgument(1, hasType(isInteger())),
           anyOf(
               // Detect the expression: string("...", 0);
               hasArgument(1, ZeroExpr.bind("empty-string")),
               // Detect the expression: string("...", -4);
               hasArgument(1, NegativeExpr.bind("negative-length")),
               // Detect the expression: string("lit", 0x1234567);
               hasArgument(1, LargeLengthExpr.bind("large-length")),
               // Detect the expression: string("lit", 5)
               allOf(hasArgument(0, ConstStrLiteral.bind("literal-with-length")),
                     hasArgument(1, ignoringParenImpCasts(
                                        integerLiteral().bind("int"))))))
           .bind("constructor"),
       this);
 
   // Check the literal string constructor with char pointer.
   // [i.e. string (const char* s);]
   Finder->addMatcher(
       traverse(TK_AsIs,
-      cxxConstructExpr(hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
-                       hasArgument(0, expr().bind("from-ptr")),
-                       hasArgument(1, unless(hasType(isInteger()))))
+               cxxConstructExpr(
+                   hasDeclaration(cxxConstructorDecl(ofClass(cxxRecordDecl(
+                       hasAnyName(removeNamespaces(StringNames)))))),
+                   // do not match std::string(ptr, int)
+                   // match std::string(ptr, alloc)
+                   // match std::string(ptr)
+                   anyOf(hasArgument(1, unless(hasType(isInteger()))),
+                         argumentCountIs(1)))
                    .bind("constructor")),
       this);
 }
 
 void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
   const ASTContext &Ctx = *Result.Context;
   const auto *E = Result.Nodes.getNodeAs<CXXConstructExpr>("constructor");
   assert(E && "missing constructor expression");
   SourceLocation Loc = E->getBeginLoc();

clang-tools-extra/docs/clang-tidy/checks/bugprone-string-constructor.rst

 Calling the string-literal constructor with a length bigger than the literal is
 suspicious and adds extra random characters to the string.
 
 Examples:
 
 .. code-block:: c++
 
   std::string("test", 200);   // Will include random characters after "test".
+  std::string_view("test", 200);
 
 Creating an empty string from constructors with parameters is considered
 suspicious. The programmer should use the empty constructor instead.
 
 Examples:
 
 .. code-block:: c++
 
   std::string("test", 0);   // Creation of an empty string.
+  std::string_view("test", 0);
 
 Options
 -------
 
 .. option::  WarnOnLargeLength
 
    When non-zero, the check will warn on a string with a length greater than
    `LargeLengthThreshold`. Default is `1`.
 
 .. option::  LargeLengthThreshold
 
    An integer specifying the large length threshold. Default is `0x800000`.
+
+.. option:: StringNames
+
+    Default is `::std::basic_string;::std::basic_string_view`.
+
+    Semicolon-delimited list of class names to apply this check to.
+    By default `::std::basic_string` applies to ``std::string`` and
+    ``std::wstring``. Set to e.g. `::std::basic_string;llvm::StringRef;QString`
+    to perform this check on custom classes.

clang-tools-extra/test/clang-tidy/checkers/bugprone-string-constructor.cpp

 // RUN: %check_clang_tidy %s bugprone-string-constructor %t
 
 namespace std {
 template <typename T>
 class allocator {};
 template <typename T>
 class char_traits {};
 template <typename C, typename T = std::char_traits<C>, typename A = std::allocator<C> >
 struct basic_string {
   basic_string();
   basic_string(const C*, unsigned int size);
   basic_string(const C *, const A &allocator = A());
   basic_string(unsigned int size, C c);
 };
 typedef basic_string<char> string;
 typedef basic_string<wchar_t> wstring;
+
+template <typename C, typename T = std::char_traits<C>>
+struct basic_string_view {
+  basic_string_view();
+  basic_string_view(const C *, unsigned int size);
+  basic_string_view(const C *);
+};
+typedef basic_string_view<char> string_view;
+typedef basic_string_view<wchar_t> wstring_view;
 }
 
 const char* kText = "";
 const char kText2[] = "";
 extern const char kText3[];
 
 void Test() {
   std::string str('x', 4);
void Test() {
   std::string q5(kText3,  0x1000000);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
   std::string q6(nullptr);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructing string from nullptr is undefined behaviour
   std::string q7 = 0;
   // CHECK-MESSAGES: [[@LINE-1]]:20: warning: constructing string from nullptr is undefined behaviour
 }
 
+void TestView() {
+  std::string_view q0("test", 0);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: constructor creating an empty string
+  std::string_view q1(kText, -4);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: negative value used as length parameter
+  std::string_view q2("test", 200);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: length is bigger than string literal size
+  std::string_view q3(kText, 200);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: length is bigger than string literal size
+  std::string_view q4(kText2, 200);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: length is bigger than string literal size
+  std::string_view q5(kText3, 0x1000000);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: suspicious large length parameter
+  std::string_view q6(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: constructing string from nullptr is undefined behaviour
+  std::string_view q7 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: constructing string from nullptr is undefined behaviour
+}
+
 std::string StringFromZero() {
   return 0;
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr is undefined behaviour
 }
 
+std::string_view StringViewFromZero() {
+  return 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr is undefined behaviour
+}
+
 void Valid() {
   std::string empty();
   std::string str(4, 'x');
   std::wstring wstr(4, L'x');
   std::string s1("test", 4);
   std::string s2("test", 3);
   std::string s3("test");
+
+  std::string_view emptyv();
+  std::string_view sv1("test", 4);
+  std::string_view sv2("test", 3);
+  std::string_view sv3("test");
 }
 
 namespace instantiation_dependent_exprs {
 template<typename T>
 struct S {
   bool x;
   std::string f() { return x ? "a" : "b"; }
+  std::string_view g() { return x ? "a" : "b"; }
 };
 }