[clang-tools-extra] 3807079 - [clang-tidy] Fix crash in readability-redundant-string-cstr

[Nathan James via cfe-commits]

Author: Nathan James
Date: 2020-03-31T13:27:32+01:00
New Revision: 3807079d705fe04c5c3bde8f848ec922b5771f15

URL: https://github.com/llvm/llvm-project/commit/3807079d705fe04c5c3bde8f848ec922b5771f15
DIFF: https://github.com/llvm/llvm-project/commit/3807079d705fe04c5c3bde8f848ec922b5771f15.diff

LOG: [clang-tidy] Fix crash in readability-redundant-string-cstr

Summary: Addresses [[ https://bugs.llvm.org/show_bug.cgi?id=45286 | clang-tidy-11: Crash in DynTypedMatcher::matches during readability-redundant-string-cstr check ]]

Reviewers: aaron.ballman, alexfh, gribozavr2

Reviewed By: gribozavr2

Subscribers: xazax.hun, cfe-commits

Tags: #clang, #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D76761

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 8975f294373c..e41cdfcc08d8 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -61,53 +61,8 @@ 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 (unsigned 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());
+AST_MATCHER(MaterializeTemporaryExpr, isBoundToLValue) {
+  return Node.isBoundToLvalueReference();
 }
 
 } // end namespace
@@ -141,11 +96,11 @@ void RedundantStringCStrCheck::registerMatchers(
   // Detect redundant 'c_str()' calls through a string constructor.
   // 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);
+  Finder->addMatcher(cxxConstructExpr(StringConstructorExpr,
+                                      hasArgument(0, StringCStrCallExpr),
+                                      unless(hasParent(materializeTemporaryExpr(
+                                          unless(isBoundToLValue()))))),
+                     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 1773dc57a8d8..2561b81805bd 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
@@ -220,3 +220,27 @@ void m1(std::string&&) {
   m1tp m1p2 = m1;
   m1p2(s.c_str());  
 }
+
+namespace PR45286 {
+struct Foo {
+  void func(const std::string &) {}
+  void func2(std::string &&) {}
+};
+
+void bar() {
+  std::string Str{"aaa"};
+  Foo Foo;
+  Foo.func(Str.c_str());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}Foo.func(Str);{{$}}
+
+  // Ensure it doesn't transform Binding to r values
+  Foo.func2(Str.c_str());
+
+  // Ensure its not confused by parens
+  Foo.func((Str.c_str()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}Foo.func((Str));{{$}}
+  Foo.func2((Str.c_str()));
+}
+} // namespace PR45286