[clang-tidy] Flag more buggy string constructor cases

[ccotter]

Summary

Add more cases to the "swapped args" case of
bugprone-string-constructor check when the first argument is
a reference to a char variable or function that returns a char.

Calls to the fill constructor where both arguments are
implicitly cast is "confusing" since the implicit cast may not be
apparent to the reader. In many cases, this is a programmer error. In
cases where it's not an error, the code should use explicit casts to
clearly specify intent.

Note that the swapped args case is a subset of the "confusing args"
case. Not all confusing args are swapped args. Consider

char buffer[] = "abcdef";
std::string not_swapped(buffer[1], 2); // oops! should be '&buffer[1]'
assert(not_swapped == "bc");

The only case where the swapped args is assumed is when the
first argument to the fill constructor is one where it is not possible
the author forgot to prefix the expression with &. I.e., when the
first arg is

• char literal
• declRefExpr to variable of type char (but not char&).
• function that returns char

With the declRefExpr case, it is possible to take the address
of a variable of char type, but only if the number of bytes
read is no more than 1:

char ch;
std::string fill(ch, 1);
// intended 'std::string fill(1, ch)' instead?
// or intended 'std::string fill(&ch, 1)'?
// both lead to the same end result anyway, but
// the former is cleaner. so swapping here is safe.

[ccotter]

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

96

This is specifically a bug I wrote recently where I meant to write &kText[1]. This doesn't fall under the "arguments swapped" case since indeed I merely forgot a '&'. At most, the tool can flag this potentially buggy code and suggest casts if the author determines the code is correct as is. However, the tool does not actually provide replacements as such code is more likely to be incorrect and require the author to determine the fix and apply it.

[ccotter]

• Add more cases to swapped params

[ccotter]

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

40

swapped[1,2,5,7], wswapped were all supported by the original logic - i.e., the ones with a literal char as the first arg.

The other swapped* cases are newly supported with my changes.

[ccotter]

• Add more cases to swapped params

[PiotrZSL]

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

70–74

add a testcase with:

char& chRef = ch;
std::string swapped2(chRef, i);

char* chPtr = &ch;
std::string swapped2(*chPtr, i);

using Character = char;
Character  c;
std::string swapped2(c, i);

I fear that it may not match those, because here you are trying to match specific use cases, when you should simply match a type of expression. In such case this should be just:

const auto CharExpr = expr(hasType(qualType(hasCanonicalType(anyOf(isAnyCharacter(), references(isAnyCharacter()))))));

Only issue would be implicit cast, you can try deal with them by using ignoringImplicit, or in better way just by using:

traverse(TK_IgnoreUnlessSpelledInSource,  hasArgument(0, CharExpr.bind("swapped-parameter"))),

TK_IgnoreUnlessSpelledInSource will cut of all implicit operations, so you could check if called constructor is actually the wrong one, and check argument types. Because now macher in line 125 may not always work if somehow we would have for example 2 implicit casts, for example from char reference to char, or some other crap.

It's up to you, but consider more generic approach instead of matching specific use cases like references to variables, or calls.

95

what about references to integer, or typedefs to integers ?

97

reuse here CharExpr

213

i thing that this case should be matched by "swapped-parameter", no need to add extra one, just first one should be fixed.

[PiotrZSL]

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

97

try with test like this:

struct Class
{
   operator char() const;
};

Class c;
std::string value(c, 5);

I fear that hasSourceExpression could match Class type, and therefore this case wouldn't be detected.

[carlosgalvezp]

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

25

Should we use C++ casts?

25

Should this be a char?

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

141

Shouldn't this fail, since the constructor std::string(char, char) does not exist?

[ccotter]

• Add more cases to swapped params
• Change message, add more tests

[ccotter]

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

70–74

Thinking about this more, I think we have to keep the "args swapped" case very narrow to the existing patterns of the first arg being a char literal, and the second being an integer literal. Consider

char buffer[] = "abcdef";
char& chRef = ch;
std::string not_swapped(chRef, 4); // Forgot '&' in front of 'chRef'

std::string also_not_swapped(buffer[2], 2); // Forgot '&' in front of 'buffer[2]'

Neither of these are swapped arg bugs, so I don't think we can have the tool consider this a swapped arg bug and apply fixits. The also_not_swapped is the exact bug I wrote a few weeks ago.

I think there are two types of bugs the tool has to be aware of with respect to the string fill constructor

1. swapped args
2. "confusing" args - where the arguments are both implicitly cast from int to char and vice versa.

For swapped args, the existing logic will match when the first arg has isInteger() type, and the second arg is characterLiteral(). At most, I think we can only expand the logic to include constructor exprs when the second arg is a declRefExpr to a char, but not char ref (see the not_swapped example above), or when the second arg is the result of a function call that returns a char. In these cases, we can be sure that the the author did not mean to put a & in front of the second argument expression.

For "confusing" args, which is not implemented in the existing logic, but I am proposing in this change, this is the more general case where I think the logic should match any expression as long as both argument expressions are implicitly cast. This is "confusing" because int-to-char and char-to-int conversions are not obvious to the reader, and it should be rare for both conversions to take place when calling the fill constructor (confirmed anecdotally in the codebases I've checked). The tool could not offer fixits here since the fix might be to swap the args, or to add a & in front of the first arg (or even another change). At most, we can warn and alert the user to the problem. If this is what the author intended, they can use explicit casts to make the intent obvious to the reader.

95

I'll add a test for reference to integer, but I'm matching on the type of the source expression that is being cast, which will be of integer type, even if the expression is string str(x, y) and y is an int& (the source expression in the cast will be of type int).

213

See earlier explanation on why we can't treat these all as swapped parameters cases.

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

25

I'm not sure what you mean here - would you mind clarifying? Do you mean should this example cast the second arg '5' to char?

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

141

The new logic I wrote flags when both parameters are implicitly cast from int to char for the first arg, and char to int in the second argument. This specific situation is what I found to be most confusing and likely bug prone. I didn't include the case where only one argument is implicitly cast to be buggy, since I wanted to be more conservative in how many more expressions to flag as being buggy, and two implicit casts (which is a bug I wrote recently) is "more" bug prone than just one. We could expand it however - would you like to explore that option?

[ccotter]

rebase

[ccotter]

rebase

[ccotter]

bump please?

[ccotter]

bump please?

[PiotrZSL]

I run into false positive with this example:

using UInt8 = unsigned char;
UInt8 size = 5U;
std::string str2(size, 'x');  //  warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]

This is due to change in CharExpr, simply isAnyCharacter is too wide, as it also match signed/unsigned characters, and your tests does not cover those.
Probably for this we need to be more strict, and accept only non signed/unsigned characters.

Other issue is with this:

char c = '\n';
using Size = int;
Size size = 10U;
std::string str2(c, size);

Even that in AST we got double implicit casts, it's detected as warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor].
Instead of being detected as 'confusing string fill constructor arguments'. This probably could be fixed by matching those double casts first, instead of last.

|     `-VarDecl 0x56436ceb0520 <col:3, col:27> col:15 str2 'std::string':'std::basic_string<char>' callinit
|       `-CXXConstructExpr 0x56436ceb0650 <col:15, col:27> 'std::string':'std::basic_string<char>' 'void (unsigned int, char)'
|         |-ImplicitCastExpr 0x56436ceb0608 <col:20> 'unsigned int' <IntegralCast>
|         | `-ImplicitCastExpr 0x56436ceb05f0 <col:20> 'char' <LValueToRValue>
|         |   `-DeclRefExpr 0x56436ceb0588 <col:20> 'char' lvalue Var 0x56436ceb0280 'c' 'char'
|         `-ImplicitCastExpr 0x56436ceb0638 <col:23> 'char':'char' <IntegralCast>
|           `-ImplicitCastExpr 0x56436ceb0620 <col:23> 'Size':'int' <LValueToRValue>
|             `-DeclRefExpr 0x56436ceb05a8 <col:23> 'Size':'int' lvalue Var 0x56436ceb0420 'size' 'Size':'int'

Except above 2 change looks OK to me. My main concern is false-positive, simply because in project that I work for we use lot of std::uint8_t as size, to save bytes in struct.

[ccotter]

• Exclude false positive

[ccotter]

Thanks, fixed the first false positive example you gave. Let me think about the second example in your most recent post:

char c = '\n';
using Size = int;
Size size = 10U;
std::string str2(c, size);

This is my newly added case swapped4, where one of my original intents was to flag this code as "swapped." I can see the the ambiguity. Worst case, "confusing string fill constructor arguments" is the fallback that I think is acceptable, and still achieves the goal of at least flagging the code as potentially buggy.

[PiotrZSL]

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

69

this may incorrectly work also for signed char, we want it work only for char and wchat_t, not signed char, unsigned char, signed wchar_t, unsigned wchar_t.
So this still need some work. I would say new "isAnyCharacter" need to be created.

95

Similar could be here, we consider std::uint8_t and std::sint8_t as integers, not as an characters, same with signed char, unsigned char

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
 //
 //===----------------------------------------------------------------------===//
 
StringConstructorCheck::StringConstructorCheck(StringRef Name,
     : ClangTidyCheck(Name, Context),
       IsStringviewNullptrCheckEnabled(
           Context->isCheckEnabled("bugprone-stringview-nullptr")),
       WarnOnLargeLength(Options.get("WarnOnLargeLength", true)),
       LargeLengthThreshold(Options.get("LargeLengthThreshold", 0x800000)),
       StringNames(utils::options::parseStringList(
           Options.get("StringNames", DefaultStringNames))) {}
 
+namespace {
+AST_MATCHER_P2(CXXConstructExpr, hasRawArgument, unsigned, N,
+               ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  if (N >= Node.getNumArgs()) {
+    return false;
+  }
+  const Expr *Arg = Node.getArg(N);
+  return InnerMatcher.matches(*Arg, Finder, Builder);
+}
+} // namespace
+
 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 CharExpr = expr(anyOf(
+      ignoringParenImpCasts(characterLiteral()),
+      declRefExpr(hasDeclaration(varDecl(hasType(qualType(isAnyCharacter()))))),
+      ignoringParens(callExpr(
+          callee(functionDecl(returns(qualType(isAnyCharacter()))))))));
   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.
   const auto BoundStringLiteral = stringLiteral().bind("str");
   const auto ConstStrLiteralDecl = varDecl(
       isDefinition(), hasType(constantArrayType()), hasType(isConstQualified()),
       hasInitializer(ignoringParenImpCasts(BoundStringLiteral)));
   const auto ConstPtrStrLiteralDecl = varDecl(
       isDefinition(),
       hasType(pointerType(pointee(isAnyCharacter(), isConstQualified()))),
       hasInitializer(ignoringParenImpCasts(BoundStringLiteral)));
   const auto ConstStrLiteral = expr(ignoringParenImpCasts(anyOf(
       BoundStringLiteral, declRefExpr(hasDeclaration(anyOf(
                               ConstPtrStrLiteralDecl, ConstStrLiteralDecl))))));
+  const auto NonCharacterInteger =
+      qualType(isInteger(), unless(isAnyCharacter()));
+  const auto CharToIntCastExpr = implicitCastExpr(
+      hasSourceExpression(expr(hasType(qualType(isAnyCharacter())))),
+      hasImplicitDestinationType(NonCharacterInteger));
+  const auto IntToCharCastExpr =
+      implicitCastExpr(hasSourceExpression(expr(hasType(NonCharacterInteger))),
+                       hasImplicitDestinationType(qualType(isAnyCharacter())));
 
   // Check the fill constructor. Fills the string with n consecutive copies of
   // character c. [i.e string(size_t n, char c);].
   Finder->addMatcher(
       cxxConstructExpr(
           hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
           hasArgument(0, hasType(qualType(isInteger()))),
           hasArgument(1, hasType(qualType(isInteger()))),
-          anyOf(
+          anyOf(anyOf(
                     // Detect the expression: string('x', 40);
                     hasArgument(0, CharExpr.bind("swapped-parameter")),
                     // Detect the expression: string(0, ...);
                     hasArgument(0, ZeroExpr.bind("empty-string")),
                     // Detect the expression: string(-4, ...);
                     hasArgument(0, NegativeExpr.bind("negative-length")),
                     // Detect the expression: string(0x1234567, ...);
-              hasArgument(0, LargeLengthExpr.bind("large-length"))))
+                    hasArgument(0, LargeLengthExpr.bind("large-length"))),
+                // Finally, check for implicitly cast of both arguments. When
+                // the source type and destination types of the implicit casts
+                // from character to integer or vice versa for both arguments,
+                // this can be confusing to readers and indicative of a bug in
+                // the program. E.g., string(ch, i) implicitly casts 'ch' to
+                // size_type and casts 'i' to char.
+                allOf(hasRawArgument(0, CharToIntCastExpr),
+                      hasRawArgument(1, IntToCharCastExpr.bind(
+                                            "implicit-cast-both-args")))))
           .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(cxxConstructorDecl(ofClass(
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();
 
   if (Result.Nodes.getNodeAs<Expr>("swapped-parameter")) {
     const Expr *P0 = E->getArg(0);
     const Expr *P1 = E->getArg(1);
-    diag(Loc, "string constructor parameters are probably swapped;"
+    diag(Loc, "string constructor arguments are probably swapped;"
               " expecting string(count, character)")
         << tooling::fixit::createReplacement(*P0, *P1, Ctx)
         << tooling::fixit::createReplacement(*P1, *P0, Ctx);
   } else if (Result.Nodes.getNodeAs<Expr>("empty-string")) {
     diag(Loc, "constructor creating an empty string");
   } else if (Result.Nodes.getNodeAs<Expr>("negative-length")) {
     diag(Loc, "negative value used as length parameter");
   } else if (Result.Nodes.getNodeAs<Expr>("large-length")) {
if (!Ptr->isInstantiationDependent() &&
       if (IsStringviewNullptrCheckEnabled &&
           Result.Nodes.getNodeAs<CXXRecordDecl>("basic_string_view_decl")) {
         // Filter out `basic_string_view` to avoid conflicts with
         // `bugprone-stringview-nullptr`
         return;
       }
       diag(Loc, "constructing string from nullptr is undefined behaviour");
     }
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("implicit-cast-both-args")) {
+    diag(Loc, "string constructor arguments might be incorrect;"
+              " calling as string(count, character) due to implicit casting of "
+              "both arguments;"
+              " use explicit casts if string(count, character) is the intended "
+              "constructor");
   }
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`bugprone-dynamic-static-initializers
   <clang-tidy/checks/bugprone/dynamic-static-initializers>` check.
   Global options of the same name should be used instead.
 
+- Updated :doc:`bugprone-string-constructor
+  <clang-tidy/checks/bugprone/string-constructor>` check to flag more potentially
+  buggy code.
+
 - Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions`
   in :doc:`bugprone-suspicious-include
   <clang-tidy/checks/bugprone/suspicious-include>` check.
   Global options of the same name should be used instead.
 
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   <clang-tidy/checks/google/build-namespaces>` check.

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

 .. title:: clang-tidy - bugprone-string-constructor
 
 bugprone-string-constructor
 ===========================
 
 Finds string constructors that are suspicious and probably errors.
 
-A common mistake is to swap parameters to the 'fill' string-constructor.
+A common mistake is to swap arguments to the 'fill' string-constructor.
 
 Examples:
 
 .. code-block:: c++
 
+  int x;
   std::string str('x', 50); // should be str(50, 'x')
+  std::string str2('a', x); // should be str2(x, 'a')
+
+Constructing a string using the 'fill' constructor where both arguments
+are implicitly cast is confusing and likely indicative of a bug.
+
+.. code-block: c++
+
+  char buf[10];
+  std::string str(buf[1], 5); // First arg should be '&buf[1]'?
+  std::string str2((int)buf[1], 5); // Ok - explicitly cast to express intent
 
 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
+Creating an empty string from constructors with arguments 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);

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> >
+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);
+  basic_string(const basic_string&, unsigned int, unsigned int, const A & = A());
+  basic_string(const C*, unsigned int);
+  basic_string(const C*, const A& = A());
+  basic_string(unsigned int, 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 *, unsigned int);
   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[];
+char getChar();
+const char* getCharPtr();
 
 void Test() {
-  std::string str('x', 4);
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor parameters are probably swapped; expecting string(count, character) [bugprone-string-constructor]
-  // CHECK-FIXES: std::string str(4, 'x');
-  std::wstring wstr(L'x', 4);
-  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor parameters are probably swapped
-  // CHECK-FIXES: std::wstring wstr(4, L'x');
+  short sh;
+  int i;
+  char ch;
+
+  std::string swapped('x', 4);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped(4, 'x');
+  std::wstring wswapped(L'x', 4);
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor arguments are probably swapped
+  // CHECK-FIXES: std::wstring wswapped(4, L'x');
+  std::string swapped2('x', i);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped2(i, 'x');
+  std::string swapped3(ch, 4);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped3(4, ch);
+  std::string swapped4(ch, i);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped4(i, ch);
+  std::string swapped5('x', (int)'x');
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped5((int)'x', 'x');
+  std::string swapped6(getChar(), 10);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped6(10, getChar());
+  std::string swapped7((('x')), ( i ));
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped7(( i ), (('x')));
+  std::string swapped8((ch), (i));
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped8((i), (ch));
+  std::string swapped9((getChar()), (i));
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+  // CHECK-FIXES: std::string swapped9((i), (getChar()));
   std::string s0(0, 'x');
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string
   std::string s1(-4, 'x');
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: negative value used as length parameter
   std::string s2(0x1000000, 'x');
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
+  std::string s3(kText[0], sh);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments might be incorrect;
+  std::string s4(kText[1], 10);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments might be incorrect;
+  std::string s5(kText[1], i);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments might be incorrect;
+  std::string s6(*getCharPtr(), 10);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments might be incorrect;
 
   std::string q0("test", 0);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string
   std::string q1(kText, -4);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: negative value used as length parameter
   std::string q2("test", 200);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger than string literal size
   std::string q3(kText, 200);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger than string literal size
   std::string q4(kText2, 200);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger than string literal size
   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);

 }
 
 std::string_view StringViewFromZero() {
   return 0;
   // CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr is undefined behaviour
 }
 
 void Valid() {
+  int i;
   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 s4((int)kText[1], i);
+  std::string s5(kText[1], (char)i);
 
   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"; }
 };
 }