[clang-tidy] Fix PR#44620 'readability-redundant-string-cstr quick-fix causes invalid code'

[f00kat]

Summary

#include <string>

static void f2(std::string&&) {
}

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

For this case I decide to do nothing by skipping it in the matcher. Another way is to fix with 'std::move(s)' but I`m not sure that it is correct.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

64

Missing a full stop at the end of the comment.

66

This function and the one below can be static.

83

Missing full stop at the end of the comment.

93

What if the call expression has no callee decl, like a call through a function pointer? This might have to be dyn_cast_or_null<> (That's probably a good test case to add.)

147

Missing a full stop at the end of the comment.

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

209–210

Missing full stop, can also remove the trailing newline.

[f00kat]

1. Add full stops at the end of the lines.
2. Add some static.
3. Fix function 'checkParamDeclOfAncestorCallExprHasRValueRefType' to work well with the function pointer calls.

[aaron.ballman]

LGTM! Do you need me to commit on your behalf, or have you obtained git privileges recently?

[f00kat]

Yes, commit please. Thank you!

[aaron.ballman]

I've commit on your behalf in 47282b1b4bf3e18d2e2166b87159115ed520a2aa, thank you for the patch!

clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp

if (Text.empty())
     return std::string();
   // Add leading '*'.
   if (needParensAfterUnaryOperator(ExprNode)) {
     return (llvm::Twine("*(") + Text + ")").str();
   }
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get CallExpr in which CxxConstructExpr is called
+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
+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 *TheCallExprDecl =
+                dyn_cast<FunctionDecl>(TheCallExpr->getCalleeDecl())) {
+          if (TheCallExprDecl->getParamDecl(i)
+                  ->getType()
+                  ->isRValueReferenceType())
+            return true;
+        }
+      }
+    }
+  }
+
+  return false;
+}
+
+AST_MATCHER(CXXConstructExpr,
+            matchedParamDeclOfAncestorCallExprHasRValueRefType) {
+  return checkParamDeclOfAncestorCallExprHasRValueRefType(
+      &Node, Finder->getASTContext());
+}
+
 } // end namespace
 
 void RedundantStringCStrCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus)
     return;
void RedundantStringCStrCheck::registerMatchers(
   // Match a call to the string 'c_str()' method.
   const auto StringCStrCallExpr =
       cxxMemberCallExpr(on(StringExpr.bind("arg")),
                         callee(memberExpr().bind("member")),
                         callee(cxxMethodDecl(hasAnyName("c_str", "data"))))
           .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())),
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(
       cxxOperatorCallExpr(
           anyOf(
               hasOverloadedOperatorName("<"), hasOverloadedOperatorName(">"),
               hasOverloadedOperatorName(">="), hasOverloadedOperatorName("<="),
               hasOverloadedOperatorName("!="), hasOverloadedOperatorName("=="),

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

struct NotAString {
   const char *c_str() const;
 };
 
 void dummy(const char*) {}
 
 void invalid(const NotAString &s) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string
+
+void m1(std::string&& s) {
+  m1(s.c_str());
+}