[clang-tidy] Fix RedundantStringCStrCheck with r values

[njames93]

Summary

The previous fix for this, https://reviews.llvm.org/D76761, Passed test cases but failed in the real world as std::string has a non trivial destructor so creates a CXXBindTemporaryExpr.

This handles that shortfall and updates the test case std::basic_string implementation to use a non trivial destructor to reflect real world behaviour.

[gribozavr2]

Passed test cases but failed in the real world as std::string has a non trivial destructor so creates a CXXBindTemporaryExpr.

An idea for a future change: move the std::string mock from this test into a header that is shared across all tests that need a std::string. That will hopefully allow us to combine forces when curating the standard library mocks.

[njames93]

In D84831#2181336, @gribozavr2 wrote:

Passed test cases but failed in the real world as std::string has a non trivial destructor so creates a CXXBindTemporaryExpr.

An idea for a future change: move the std::string mock from this test into a header that is shared across all tests that need a std::string. That will hopefully allow us to combine forces when curating the standard library mocks.

That does sound like a good plan, could also add maybe vector

Also a future change for this specific check would be to replace the call to c_str() when its being bound to an r value with an explicit copy constructor call

 void foo(std::string &&);
 void bar(const std::string& S) {
-  foo(S.c_str());
+  foo(std::string(S));
 }

This would be more of a performance change though, basically save a call to strlen.
Though arguably it is slightly more readable, as you are explicitly saying you want to pass a copy of the string

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

const auto StringConstructorExpr = expr(anyOf(
           hasArgument(1, cxxDefaultArgExpr()))));
 
   // 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");
-
+  const auto HasRValueTempParent =
+      hasParent(materializeTemporaryExpr(unless(isBoundToLValue())));
   // 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(
-      traverse(ast_type_traits::TK_AsIs,
-               cxxConstructExpr(StringConstructorExpr,
-                                hasArgument(0, StringCStrCallExpr),
-                                unless(hasParent(materializeTemporaryExpr(
-                                    unless(isBoundToLValue())))))),
+      traverse(
+          ast_type_traits::TK_AsIs,
+          cxxConstructExpr(
+              StringConstructorExpr, hasArgument(0, StringCStrCallExpr),
+              unless(anyOf(HasRValueTempParent, hasParent(cxxBindTemporaryExpr(
+                                                    HasRValueTempParent)))))),
       this);
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(
       cxxOperatorCallExpr(
           hasAnyOverloadedOperatorName("<", ">", ">=", "<=", "!=", "==", "+"),
           anyOf(allOf(hasArgument(0, StringExpr),
                       hasArgument(1, StringCStrCallExpr)),

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

 template <typename T>
 class char_traits {};
 template <typename C, typename T, typename A>
 struct basic_string {
   typedef basic_string<C, T, A> _Type;
   basic_string();
   basic_string(const C *p, const A &a = A());
 
+  ~basic_string();
+
   const C *c_str() const;
   const C *data() const;
 
   _Type& append(const C *s);
   _Type& append(const C *s, size n);
   _Type& assign(const C *s);
   _Type& assign(const C *s, size n);