[clang-tidy] Add FixIt for swapping arguments in string-constructor-checker.

[etienneb]

Summary

Arguments can be swapped using fixit when they are not in macros.
This is the same implementation than SwappedArguments. Some code
got lifted to be reused.

Others checks are not safe to be fixed as they tend to be bugs or errors.
It is better to let the user manually review them.

[alexfh]

clang-tidy/misc/StringConstructorCheck.cpp

110–115

Assertions should verify logical bugs in the code, not some properties of the user input. The matcher doesn't seem to ensure that the argument count is always 2, so it's possible to create code that will trigger this assertion. Either add argumentCountIs(2) to the matcher or remove the assertion.

[etienneb]

address alexfh comments.

clang-tidy/misc/StringConstructorCheck.cpp

110–115

As I get it, it's not needed to add ' argumentCountIs(2)' because:

hasArgument(0, hasType(CharPtrType)),
hasArgument(1, hasType(isInteger())),

the code already validates that there is at least two parameters.
The assert would fail if there is more parameters, which can occur only on a incorrect definition of the constructor.

So, I'm i favor to just remove the assert.

[alexfh]

clang-tidy/misc/StringConstructorCheck.cpp

110–115

We could make the message slightly more specific by saying it's std::(basic_)?string constructor.

115

You should be able to use Lexer::getSourceText instead.

[etienneb]

use new API. code simplification

[alexfh]

LG.

clang-tidy/misc/StringConstructorCheck.cpp

 //===--- StringConstructorCheck.cpp - clang-tidy---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "StringConstructorCheck.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 misc {
 
 AST_MATCHER_P(IntegerLiteral, isBiggerThan, unsigned, N) {
void StringConstructorCheck::registerMatchers(MatchFinder *Finder) {
   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)));
-  auto ConstStrLiteral = expr(ignoringParenImpCasts(anyOf(
+  const auto ConstStrLiteral = expr(ignoringParenImpCasts(anyOf(
       BoundStringLiteral, declRefExpr(hasDeclaration(anyOf(
                               ConstPtrStrLiteralDecl, ConstStrLiteralDecl))))));
 
   // 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"))),
Finder->addMatcher(
       cxxConstructExpr(
           hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
           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")),              
+              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);
 }
 
 void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *E = Result.Nodes.getNodeAs<Expr>("constructor");
+  const ASTContext &Ctx = *Result.Context;
+  const auto *E = Result.Nodes.getNodeAs<CXXConstructExpr>("constructor");
+  assert(E && "missing constructor expression");
   SourceLocation Loc = E->getLocStart();
 
   if (Result.Nodes.getNodeAs<Expr>("swapped-parameter")) {
-    diag(Loc, "constructor parameters are probably swapped");
+    const Expr* P0 = E->getArg(0);
+    const Expr* P1 = E->getArg(1);
+    diag(Loc, "string constructor parameters 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 (WarnOnLargeLength)
       diag(Loc, "suspicious large length parameter");
   } else if (Result.Nodes.getNodeAs<Expr>("literal-with-length")) {

test/clang-tidy/misc-string-constructor.cpp

 }
 
 const char* kText = "";
 const char kText2[] = "";
 extern const char kText3[];
 
 void Test() {
   std::string str('x', 4);
-  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor parameters are probably swapped [misc-string-constructor]
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor parameters are probably swapped; expecting string(count, character) [misc-string-constructor]
+  // CHECK-FIXES: std::string str(4, 'x');  
   std::wstring wstr(L'x', 4);
-  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: constructor parameters are probably swapped
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor parameters are probably swapped
+  // CHECK-FIXES: std::wstring wstr(4, L'x');    
   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 q0("test", 0);