[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–123

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–123

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

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 "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
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");
+    auto D = diag(Loc, "constructor parameters are probably swapped");
+
+    SourceRange Param1 = E->getArg(0)->getSourceRange();
+    SourceRange Param2 = E->getArg(1)->getSourceRange();
+    StringRef Param1String =
+        utils::create_fix_it::getSourceRangeAsString(Ctx, Param1);
+    StringRef Param2String =
+        utils::create_fix_it::getSourceRangeAsString(Ctx, Param2);
+    if (!Param1String.empty() && !Param2String.empty()) {
+      D << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(Param1),
+                                        Param2String)
+        << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(Param2),
+                                        Param1String);
+    }
   } 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")) {

clang-tidy/misc/SwappedArgumentsCheck.cpp

 //===--- SwappedArgumentsCheck.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 "SwappedArgumentsCheck.h"
+#include "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
return Cast->getCastKind() == CK_UserDefinedConversion ||
          Cast->getCastKind() == CK_FloatingToBoolean ||
          Cast->getCastKind() == CK_FloatingToIntegral ||
          Cast->getCastKind() == CK_IntegralToBoolean ||
          Cast->getCastKind() == CK_IntegralToFloating ||
          Cast->getCastKind() == CK_MemberPointerToBoolean ||
          Cast->getCastKind() == CK_PointerToBoolean;
 }
 
-/// \brief Get a StringRef representing a SourceRange.
-static StringRef getAsString(const MatchFinder::MatchResult &Result,
-                             SourceRange R) {
-  const SourceManager &SM = *Result.SourceManager;
-  // Don't even try to resolve macro or include contraptions. Not worth emitting
-  // a fixit for.
-  if (R.getBegin().isMacroID() ||
-      !SM.isWrittenInSameFile(R.getBegin(), R.getEnd()))
-    return StringRef();
-
-  const char *Begin = SM.getCharacterData(R.getBegin());
-  const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken(
-      R.getEnd(), 0, SM, Result.Context->getLangOpts()));
-
-  return StringRef(Begin, End - Begin);
-}
-
 void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) {
+  const ASTContext &Ctx = *Result.Context;
   auto *Call = Result.Nodes.getStmtAs<CallExpr>("call");
 
   llvm::SmallPtrSet<const Expr *, 4> UsedArgs;
   for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) {
     const Expr *LHS = Call->getArg(I - 1);
     const Expr *RHS = Call->getArg(I);
 
     // Only need to check RHS, as LHS has already been covered. We don't want to
SourceRange LHSRange = LHS->getSourceRange(),
                 RHSRange = RHS->getSourceRange();
     auto D =
         diag(Call->getLocStart(), "argument with implicit conversion from %0 "
                                   "to %1 followed by argument converted from "
                                   "%2 to %3, potentially swapped arguments.")
         << LHS->getType() << LHSFrom->getType() << RHS->getType()
         << RHSFrom->getType() << LHSRange << RHSRange;
 
-    StringRef RHSString = getAsString(Result, RHSRange);
-    StringRef LHSString = getAsString(Result, LHSRange);
+    StringRef RHSString =
+        utils::create_fix_it::getSourceRangeAsString(Ctx, RHSRange);
+    StringRef LHSString =
+        utils::create_fix_it::getSourceRangeAsString(Ctx, LHSRange);
     if (!LHSString.empty() && !RHSString.empty()) {
       D << FixItHint::CreateReplacement(
                CharSourceRange::getTokenRange(LHSRange), RHSString)
         << FixItHint::CreateReplacement(
                CharSourceRange::getTokenRange(RHSRange), LHSString);
     }
 
     // Remember that we emitted a warning for this argument.
     UsedArgs.insert(RHSCast);
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tidy/utils/FixItHintUtils.h

 namespace create_fix_it {
 
 /// \brief Creates fix to make VarDecl a reference by adding '&'.
 FixItHint changeVarDeclToReference(const VarDecl &Var, ASTContext &Context);
 
 /// \brief Creates fix to make VarDecl const qualified.
 FixItHint changeVarDeclToConst(const VarDecl &Var);
 
+/// \brief Get a StringRef representing a SourceRange.
+StringRef getSourceRangeAsString(const ASTContext &Context, SourceRange R);
+
 } // namespace create_fix_it
 } // utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_FIXITHINTUTILS_H

clang-tidy/utils/FixItHintUtils.cpp

AmpLocation = Lexer::getLocForEndOfToken(Token.getLocation(), 0,
                                              Context.getLangOpts());
   return FixItHint::CreateInsertion(AmpLocation, "&");
 }
 
 FixItHint changeVarDeclToConst(const VarDecl &Var) {
   return FixItHint::CreateInsertion(Var.getTypeSpecStartLoc(), "const ");
 }
 
+StringRef getSourceRangeAsString(const ASTContext &Context, SourceRange R) {
+  const SourceManager &SM = Context.getSourceManager();
+  // Don't even try to resolve macro or include contraptions. Not worth emitting
+  // a fixit for.
+  if (R.getBegin().isMacroID() ||
+      !SM.isWrittenInSameFile(R.getBegin(), R.getEnd()))
+    return StringRef();
+
+  const char *Begin = SM.getCharacterData(R.getBegin());
+  const char *End = SM.getCharacterData(
+      Lexer::getLocForEndOfToken(R.getEnd(), 0, SM, Context.getLangOpts()));
+
+  return StringRef(Begin, End - Begin);
+}
+
 } // namespace create_fix_it
 } // namespace utils
 } // namespace tidy
 } // namespace clang

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-FIXES: std::string str(4, 'x');  
   std::wstring wstr(L'x', 4);
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: 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);