[clang-tools-extra] 47282b1 - Fix PR#44620 'readability-redundant-string-cstr quick-fix causes invalid code'

[Aaron Ballman via cfe-commits]

Author: Karasev Nikita
Date: 2020-02-18T15:33:52-05:00
New Revision: 47282b1b4bf3e18d2e2166b87159115ed520a2aa

URL: https://github.com/llvm/llvm-project/commit/47282b1b4bf3e18d2e2166b87159115ed520a2aa
DIFF: https://github.com/llvm/llvm-project/commit/47282b1b4bf3e18d2e2166b87159115ed520a2aa.diff

LOG: Fix PR#44620 'readability-redundant-string-cstr quick-fix causes invalid code'

static void f2(std::string&&) {}
static void f() {
	std::string const s;
	f2(s.c_str()); // readability-redundant-string-cstr previously warning
}

Skips the problematic AST pattern in the matcher.

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
    clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
index 78834914a5cc..d365bbbe3c43 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -61,6 +61,55 @@ formatDereference(const ast_matchers::MatchFinder::MatchResult &Result,
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get CallExpr in which CxxConstructExpr is called.
+static const clang::CallExpr *
+tryGetCallExprAncestorForCxxConstructExpr(const Expr *TheExpr,
+                                          ASTContext &Context) {
+  // We skip nodes such as CXXBindTemporaryExpr, MaterializeTemporaryExpr.
+  for (ast_type_traits::DynTypedNode DynParent : Context.getParents(*TheExpr)) {
+    if (const auto *Parent = DynParent.get<Expr>()) {
+      if (const auto *TheCallExpr = dyn_cast<CallExpr>(Parent))
+        return TheCallExpr;
+
+      if (const clang::CallExpr *TheCallExpr =
+              tryGetCallExprAncestorForCxxConstructExpr(Parent, Context))
+        return TheCallExpr;
+    }
+  }
+
+  return nullptr;
+}
+
+// Check that ParamDecl of CallExprDecl has rvalue type.
+static bool checkParamDeclOfAncestorCallExprHasRValueRefType(
+    const Expr *TheCxxConstructExpr, ASTContext &Context) {
+  if (const clang::CallExpr *TheCallExpr =
+          tryGetCallExprAncestorForCxxConstructExpr(TheCxxConstructExpr,
+                                                    Context)) {
+    for (int i = 0; i < TheCallExpr->getNumArgs(); ++i) {
+      const Expr *Arg = TheCallExpr->getArg(i);
+      if (Arg->getSourceRange() == TheCxxConstructExpr->getSourceRange()) {
+        if (const auto *TheCallExprFuncProto =
+                TheCallExpr->getCallee()
+                    ->getType()
+                    ->getPointeeType()
+                    ->getAs<FunctionProtoType>()) {
+          if (TheCallExprFuncProto->getParamType(i)->isRValueReferenceType())
+            return true;
+        }
+      }
+    }
+  }
+
+  return false;
+}
+
+AST_MATCHER(CXXConstructExpr,
+            matchedParamDeclOfAncestorCallExprHasRValueRefType) {
+  return checkParamDeclOfAncestorCallExprHasRValueRefType(
+      &Node, Finder->getASTContext());
+}
+
 } // end namespace
 
 void RedundantStringCStrCheck::registerMatchers(
@@ -95,9 +144,13 @@ void RedundantStringCStrCheck::registerMatchers(
           .bind("call");
 
   // Detect redundant 'c_str()' calls through a string constructor.
-  Finder->addMatcher(cxxConstructExpr(StringConstructorExpr,
-                                      hasArgument(0, StringCStrCallExpr)),
-                     this);
+  // If CxxConstructExpr is the part of some CallExpr we need to
+  // check that matched ParamDecl of the ancestor CallExpr is not rvalue.
+  Finder->addMatcher(
+      cxxConstructExpr(
+          StringConstructorExpr, hasArgument(0, StringCStrCallExpr),
+          unless(matchedParamDeclOfAncestorCallExprHasRValueRefType())),
+      this);
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
index d8434d3ca7c5..1773dc57a8d8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp
@@ -205,3 +205,18 @@ void dummy(const char*) {}
 void invalid(const NotAString &s) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string.
+void m1(std::string&&) {
+  std::string s;
+
+  m1(s.c_str());
+
+  void (*m1p1)(std::string&&);
+  m1p1 = m1;
+  m1p1(s.c_str());
+
+  using m1tp = void (*)(std::string &&);
+  m1tp m1p2 = m1;
+  m1p2(s.c_str());  
+}