Merge topic 'clang-tidy-module-string-concatenation-cmstrcat-check'

5ad111e595 clang-tidy: disable string concatenation check
c6c8616468 clang-tidy module: add tests for string concatenation check
e1ec052d53 clang-tidy module: add check for string concatenation

Acked-by: Kitware Robot <[email protected]>
Acked-by: Ben Boeckel <[email protected]>
Merge-request: !7961

.clang-tidy

@@ -44,6 +44,7 @@ readability-*,\
 -readability-uppercase-literal-suffix,\
 cmake-*,\
 -cmake-ostringstream-use-cmstrcat,\
+-cmake-string-concatenation-use-cmstrcat,\
 -cmake-use-bespoke-enum-class,\
 "
 HeaderFilterRegex: 'Source/cm[^/]*\.(h|hxx|cxx)$'

Utilities/ClangTidyModule/CMakeLists.txt

@@ -16,6 +16,8 @@ add_library(cmake-clang-tidy-module MODULE
 
   OstringstreamUseCmstrcatCheck.cxx
   OstringstreamUseCmstrcatCheck.h
+  StringConcatenationUseCmstrcatCheck.cxx
+  StringConcatenationUseCmstrcatCheck.h
   UseBespokeEnumClassCheck.cxx
   UseBespokeEnumClassCheck.h
   UseCmstrlenCheck.cxx

Utilities/ClangTidyModule/Module.cxx

@@ -4,6 +4,7 @@
 #include <clang-tidy/ClangTidyModuleRegistry.h>
 
 #include "OstringstreamUseCmstrcatCheck.h"
+#include "StringConcatenationUseCmstrcatCheck.h"
 #include "UseBespokeEnumClassCheck.h"
 #include "UseCmstrlenCheck.h"
 #include "UseCmsysFstreamCheck.h"
@@ -25,6 +26,8 @@ public:
     CheckFactories.registerCheck<OstringstreamUseCmstrcatCheck>(
       "cmake-ostringstream-use-cmstrcat");
     CheckFactories.registerCheck<UsePragmaOnceCheck>("cmake-use-pragma-once");
+    CheckFactories.registerCheck<StringConcatenationUseCmstrcatCheck>(
+      "cmake-string-concatenation-use-cmstrcat");
   }
 };
 

Utilities/ClangTidyModule/StringConcatenationUseCmstrcatCheck.cxx

@@ -0,0 +1,179 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file Copyright.txt or https://cmake.org/licensing for details.  */
+#include "StringConcatenationUseCmstrcatCheck.h"
+
+#include <cassert>
+
+#include <clang/ASTMatchers/ASTMatchFinder.h>
+#include <clang/Lex/Lexer.h>
+
+namespace clang {
+namespace tidy {
+namespace cmake {
+using namespace ast_matchers;
+
+StringConcatenationUseCmstrcatCheck::StringConcatenationUseCmstrcatCheck(
+  StringRef Name, ClangTidyContext* Context)
+  : ClangTidyCheck(Name, Context)
+{
+}
+
+void StringConcatenationUseCmstrcatCheck::registerMatchers(MatchFinder* Finder)
+{
+  auto IsString = expr(hasType(qualType(hasUnqualifiedDesugaredType(
+    recordType(hasDeclaration(classTemplateSpecializationDecl(
+      hasName("::std::basic_string"),
+      hasTemplateArgument(
+        0, templateArgument(refersToType(asString("char")))))))))));
+
+  auto IsChar = expr(hasType(asString("char")));
+
+  auto IsCharPtr = expr(hasType(pointerType(pointee(asString("const char")))));
+
+  auto IsStringConcat =
+    cxxOperatorCallExpr(hasOperatorName("+"),
+                        anyOf(allOf(hasLHS(IsString), hasRHS(IsString)),
+                              allOf(hasLHS(IsString), hasRHS(IsChar)),
+                              allOf(hasLHS(IsString), hasRHS(IsCharPtr)),
+                              allOf(hasLHS(IsChar), hasRHS(IsString)),
+                              allOf(hasLHS(IsCharPtr), hasRHS(IsString))));
+
+  auto IsStringAppend = cxxOperatorCallExpr(
+    hasOperatorName("+="), hasLHS(IsString),
+    anyOf(hasRHS(IsString), hasRHS(IsChar), hasRHS(IsCharPtr)));
+
+  auto IsStringConcatWithLHS =
+    cxxOperatorCallExpr(
+      IsStringConcat,
+      optionally(hasLHS(materializeTemporaryExpr(
+        has(cxxBindTemporaryExpr(has(IsStringConcat.bind("lhs"))))))))
+      .bind("concat");
+
+  auto IsStringAppendWithRHS =
+    cxxOperatorCallExpr(
+      IsStringAppend,
+      optionally(hasRHS(materializeTemporaryExpr(has(implicitCastExpr(
+        has(cxxBindTemporaryExpr(has(IsStringConcat.bind("rhs"))))))))))
+      .bind("append");
+
+  Finder->addMatcher(IsStringConcatWithLHS, this);
+  Finder->addMatcher(IsStringAppendWithRHS, this);
+}
+
+void StringConcatenationUseCmstrcatCheck::check(
+  const MatchFinder::MatchResult& Result)
+{
+  const CXXOperatorCallExpr* AppendNode =
+    Result.Nodes.getNodeAs<CXXOperatorCallExpr>("append");
+  const CXXOperatorCallExpr* ConcatNode =
+    Result.Nodes.getNodeAs<CXXOperatorCallExpr>("concat");
+
+  if (AppendNode != nullptr) {
+    if (AppendNode->getBeginLoc().isValid()) {
+      assert(InProgressExprChains.find(AppendNode) ==
+             InProgressExprChains.end());
+
+      ExprChain TmpExprChain =
+        std::make_pair(OperatorType::PlusEquals,
+                       std::vector<const CXXOperatorCallExpr*>{ AppendNode });
+      const CXXOperatorCallExpr* RHSNode =
+        Result.Nodes.getNodeAs<CXXOperatorCallExpr>("rhs");
+
+      if (RHSNode != nullptr) {
+        if (RHSNode->getBeginLoc().isValid()) {
+          InProgressExprChains[RHSNode] = std::move(TmpExprChain);
+        }
+      } else {
+        issueCorrection(TmpExprChain, Result);
+      }
+    }
+  }
+
+  if (ConcatNode != nullptr) {
+    if (ConcatNode->getBeginLoc().isValid()) {
+      ExprChain TmpExprChain;
+
+      if (!(InProgressExprChains.find(ConcatNode) ==
+            InProgressExprChains.end())) {
+        TmpExprChain = std::move(InProgressExprChains[ConcatNode]);
+        InProgressExprChains.erase(ConcatNode);
+        if (TmpExprChain.first == OperatorType::PlusEquals) {
+          TmpExprChain.second.insert(TmpExprChain.second.begin() + 1,
+                                     ConcatNode);
+        } else {
+          TmpExprChain.second.insert(TmpExprChain.second.begin(), ConcatNode);
+        }
+      } else {
+        TmpExprChain = std::make_pair(
+          OperatorType::Plus,
+          std::vector<const CXXOperatorCallExpr*>{ ConcatNode });
+      }
+
+      const CXXOperatorCallExpr* LHSNode =
+        Result.Nodes.getNodeAs<CXXOperatorCallExpr>("lhs");
+
+      if (LHSNode != nullptr) {
+        if (LHSNode->getBeginLoc().isValid()) {
+          InProgressExprChains[LHSNode] = std::move(TmpExprChain);
+        }
+      } else {
+        issueCorrection(TmpExprChain, Result);
+      }
+    }
+  }
+}
+
+void StringConcatenationUseCmstrcatCheck::issueCorrection(
+  const ExprChain& Chain, const MatchFinder::MatchResult& Result)
+{
+  std::vector<FixItHint> FixIts;
+  const CXXOperatorCallExpr* ExprNode;
+  std::vector<const clang::CXXOperatorCallExpr*>::const_iterator It =
+    Chain.second.begin();
+
+  if (Chain.first == OperatorType::PlusEquals) {
+    ExprNode = *It;
+    StringRef LHS = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(ExprNode->getArg(0)->getSourceRange()),
+      Result.Context->getSourceManager(), Result.Context->getLangOpts());
+
+    FixIts.push_back(FixItHint::CreateReplacement(
+      ExprNode->getExprLoc(), "= cmStrCat(" + LHS.str() + ","));
+    It++;
+  } else {
+    ExprNode = *It;
+    FixIts.push_back(
+      FixItHint::CreateInsertion(ExprNode->getBeginLoc(), "cmStrCat("));
+  }
+
+  while (It != std::end(Chain.second)) {
+    ExprNode = *It;
+    FixIts.push_back(
+      FixItHint::CreateReplacement(ExprNode->getOperatorLoc(), ","));
+    It++;
+  }
+  It--;
+  ExprNode = *It;
+
+  StringRef LastToken = Lexer::getSourceText(
+    CharSourceRange::getTokenRange(ExprNode->getArg(1)->getSourceRange()),
+    Result.Context->getSourceManager(), Result.Context->getLangOpts());
+  FixIts.push_back(FixItHint::CreateInsertion(
+    ExprNode->getEndLoc().getLocWithOffset(LastToken.str().size()), ")"));
+
+  It = Chain.second.begin();
+  ExprNode = *It;
+
+  if (Chain.first == OperatorType::PlusEquals) {
+    this->diag(ExprNode->getOperatorLoc(),
+               "use cmStrCat() instead of string append")
+      << FixIts;
+  } else {
+    this->diag(ExprNode->getBeginLoc(),
+               "use cmStrCat() instead of string concatenation")
+      << FixIts;
+  }
+}
+}
+}
+}

Utilities/ClangTidyModule/StringConcatenationUseCmstrcatCheck.h

@@ -0,0 +1,34 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file Copyright.txt or https://cmake.org/licensing for details.  */
+#pragma once
+
+#include <clang-tidy/ClangTidyCheck.h>
+#include <clang/ASTMatchers/ASTMatchFinder.h>
+
+namespace clang {
+namespace tidy {
+namespace cmake {
+class StringConcatenationUseCmstrcatCheck : public ClangTidyCheck
+{
+public:
+  StringConcatenationUseCmstrcatCheck(StringRef Name,
+                                      ClangTidyContext* Context);
+  void registerMatchers(ast_matchers::MatchFinder* Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult& Result) override;
+
+private:
+  enum class OperatorType
+  {
+    Plus,
+    PlusEquals
+  };
+  typedef std::pair<OperatorType, std::vector<const CXXOperatorCallExpr*>>
+    ExprChain;
+  std::map<const CXXOperatorCallExpr*, ExprChain> InProgressExprChains;
+
+  void issueCorrection(const ExprChain& ExprChain,
+                       const ast_matchers::MatchFinder::MatchResult& Result);
+};
+}
+}
+}

Utilities/ClangTidyModule/Tests/CMakeLists.txt

@@ -15,3 +15,4 @@ add_run_clang_tidy_test(cmake-use-cmsys-fstream)
 add_run_clang_tidy_test(cmake-use-bespoke-enum-class)
 add_run_clang_tidy_test(cmake-ostringstream-use-cmstrcat)
 add_run_clang_tidy_test(cmake-use-pragma-once)
+add_run_clang_tidy_test(cmake-string-concatenation-use-cmstrcat)

Utilities/ClangTidyModule/Tests/cmake-string-concatenation-use-cmstrcat-fixit.cxx

@@ -0,0 +1,36 @@
+#include <string>
+
+template <typename... Args>
+std::string cmStrCat(Args&&... args)
+{
+  return "";
+}
+
+std::string a = "This is a string variable";
+std::string b = " and this is a string variable";
+std::string concat;
+
+// Correction needed
+void test1()
+{
+  concat = cmStrCat(a, b);
+  concat = cmStrCat(a, " and this is a string literal");
+  concat = cmStrCat(a, 'O');
+  concat = cmStrCat("This is a string literal", b);
+  concat = cmStrCat('O', a);
+  concat = cmStrCat(a, " and this is a string literal", 'O', b);
+
+  concat = cmStrCat(concat, b);
+  concat = cmStrCat(concat, " and this is a string literal");
+  concat = cmStrCat(concat, 'o');
+  concat = cmStrCat(concat, b, " and this is a string literal ", 'o', b);
+}
+
+// No correction needed
+void test2()
+{
+  a = b;
+  a = "This is a string literal";
+  a = 'X';
+  cmStrCat(a, b);
+}

Utilities/ClangTidyModule/Tests/cmake-string-concatenation-use-cmstrcat-stdout.txt

@@ -0,0 +1,113 @@
+cmake-string-concatenation-use-cmstrcat.cxx:16:12: warning: use cmStrCat() instead of string concatenation [cmake-string-concatenation-use-cmstrcat]
+  concat = a + b;
+           ^ ~
+           cmStrCat( , )
+cmake-string-concatenation-use-cmstrcat.cxx:16:12: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:16:14: note: FIX-IT applied suggested code changes
+  concat = a + b;
+             ^
+cmake-string-concatenation-use-cmstrcat.cxx:16:17: note: FIX-IT applied suggested code changes
+  concat = a + b;
+                ^
+cmake-string-concatenation-use-cmstrcat.cxx:17:12: warning: use cmStrCat() instead of string concatenation [cmake-string-concatenation-use-cmstrcat]
+  concat = a + " and this is a string literal";
+           ^ ~
+           cmStrCat( ,                        )
+cmake-string-concatenation-use-cmstrcat.cxx:17:12: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:17:14: note: FIX-IT applied suggested code changes
+  concat = a + " and this is a string literal";
+             ^
+cmake-string-concatenation-use-cmstrcat.cxx:17:47: note: FIX-IT applied suggested code changes
+  concat = a + " and this is a string literal";
+                                              ^
+cmake-string-concatenation-use-cmstrcat.cxx:18:12: warning: use cmStrCat() instead of string concatenation [cmake-string-concatenation-use-cmstrcat]
+  concat = a + 'O';
+           ^ ~
+           cmStrCat( , )
+cmake-string-concatenation-use-cmstrcat.cxx:18:12: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:18:14: note: FIX-IT applied suggested code changes
+  concat = a + 'O';
+             ^
+cmake-string-concatenation-use-cmstrcat.cxx:18:19: note: FIX-IT applied suggested code changes
+  concat = a + 'O';
+                  ^
+cmake-string-concatenation-use-cmstrcat.cxx:19:12: warning: use cmStrCat() instead of string concatenation [cmake-string-concatenation-use-cmstrcat]
+  concat = "This is a string literal" + b;
+           ^                          ~
+           cmStrCat(                  ,  )
+cmake-string-concatenation-use-cmstrcat.cxx:19:12: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:19:39: note: FIX-IT applied suggested code changes
+  concat = "This is a string literal" + b;
+                                      ^
+cmake-string-concatenation-use-cmstrcat.cxx:19:42: note: FIX-IT applied suggested code changes
+  concat = "This is a string literal" + b;
+                                         ^
+cmake-string-concatenation-use-cmstrcat.cxx:20:12: warning: use cmStrCat() instead of string concatenation [cmake-string-concatenation-use-cmstrcat]
+  concat = 'O' + a;
+           ^   ~
+           cmStrCat( , )
+cmake-string-concatenation-use-cmstrcat.cxx:20:12: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:20:16: note: FIX-IT applied suggested code changes
+  concat = 'O' + a;
+               ^
+cmake-string-concatenation-use-cmstrcat.cxx:20:19: note: FIX-IT applied suggested code changes
+  concat = 'O' + a;
+                  ^
+cmake-string-concatenation-use-cmstrcat.cxx:21:12: warning: use cmStrCat() instead of string concatenation [cmake-string-concatenation-use-cmstrcat]
+  concat = a + " and this is a string literal" + 'O' + b;
+           ^ ~                                 ~     ~
+           cmStrCat( ,                         ,     ,  )
+cmake-string-concatenation-use-cmstrcat.cxx:21:12: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:21:14: note: FIX-IT applied suggested code changes
+  concat = a + " and this is a string literal" + 'O' + b;
+             ^
+cmake-string-concatenation-use-cmstrcat.cxx:21:48: note: FIX-IT applied suggested code changes
+  concat = a + " and this is a string literal" + 'O' + b;
+                                               ^
+cmake-string-concatenation-use-cmstrcat.cxx:21:54: note: FIX-IT applied suggested code changes
+  concat = a + " and this is a string literal" + 'O' + b;
+                                                     ^
+cmake-string-concatenation-use-cmstrcat.cxx:21:57: note: FIX-IT applied suggested code changes
+  concat = a + " and this is a string literal" + 'O' + b;
+                                                        ^
+cmake-string-concatenation-use-cmstrcat.cxx:23:10: warning: use cmStrCat() instead of string append [cmake-string-concatenation-use-cmstrcat]
+  concat += b;
+         ^~
+         = cmStrCat(concat, )
+cmake-string-concatenation-use-cmstrcat.cxx:23:10: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:23:14: note: FIX-IT applied suggested code changes
+  concat += b;
+             ^
+cmake-string-concatenation-use-cmstrcat.cxx:24:10: warning: use cmStrCat() instead of string append [cmake-string-concatenation-use-cmstrcat]
+  concat += " and this is a string literal";
+         ^~
+         = cmStrCat(concat,                )
+cmake-string-concatenation-use-cmstrcat.cxx:24:10: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:24:44: note: FIX-IT applied suggested code changes
+  concat += " and this is a string literal";
+                                           ^
+cmake-string-concatenation-use-cmstrcat.cxx:25:10: warning: use cmStrCat() instead of string append [cmake-string-concatenation-use-cmstrcat]
+  concat += 'o';
+         ^~
+         = cmStrCat(concat, )
+cmake-string-concatenation-use-cmstrcat.cxx:25:10: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:25:16: note: FIX-IT applied suggested code changes
+  concat += 'o';
+               ^
+cmake-string-concatenation-use-cmstrcat.cxx:26:10: warning: use cmStrCat() instead of string append [cmake-string-concatenation-use-cmstrcat]
+  concat += b + " and this is a string literal " + 'o' + b;
+         ^~   ~                                  ~     ~
+         = cmStrCat(concat, ,                    ,     ,  )
+cmake-string-concatenation-use-cmstrcat.cxx:26:10: note: FIX-IT applied suggested code changes
+cmake-string-concatenation-use-cmstrcat.cxx:26:15: note: FIX-IT applied suggested code changes
+  concat += b + " and this is a string literal " + 'o' + b;
+              ^
+cmake-string-concatenation-use-cmstrcat.cxx:26:50: note: FIX-IT applied suggested code changes
+  concat += b + " and this is a string literal " + 'o' + b;
+                                                 ^
+cmake-string-concatenation-use-cmstrcat.cxx:26:56: note: FIX-IT applied suggested code changes
+  concat += b + " and this is a string literal " + 'o' + b;
+                                                       ^
+cmake-string-concatenation-use-cmstrcat.cxx:26:59: note: FIX-IT applied suggested code changes
+  concat += b + " and this is a string literal " + 'o' + b;
+                                                          ^

Utilities/ClangTidyModule/Tests/cmake-string-concatenation-use-cmstrcat.cxx

@@ -0,0 +1,36 @@
+#include <string>
+
+template <typename... Args>
+std::string cmStrCat(Args&&... args)
+{
+  return "";
+}
+
+std::string a = "This is a string variable";
+std::string b = " and this is a string variable";
+std::string concat;
+
+// Correction needed
+void test1()
+{
+  concat = a + b;
+  concat = a + " and this is a string literal";
+  concat = a + 'O';
+  concat = "This is a string literal" + b;
+  concat = 'O' + a;
+  concat = a + " and this is a string literal" + 'O' + b;
+
+  concat += b;
+  concat += " and this is a string literal";
+  concat += 'o';
+  concat += b + " and this is a string literal " + 'o' + b;
+}
+
+// No correction needed
+void test2()
+{
+  a = b;
+  a = "This is a string literal";
+  a = 'X';
+  cmStrCat(a, b);
+}