[clang-tidy] Fix readability-redundant-string-cstr for smart pointer #576705

[mikecrowe]

Summary

Fix the readability-redundant-string-cstr check to correctly replace
calls to c_str() via an overloaded operator-> (such as from an
iterator.)

Previously, the fix for i->c_str() would be *i->. Using consume_back
to remove any trailing -> results in the correct *i.

Add some lit check test cases too.

Fixes: https://github.com/llvm/llvm-project/issues/56705

[PiotrZSL]

Also consider reducing commit message, instead just copying issue description.
Simple description about issue would be sufficient.

No functional issues, so LGTM.

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

56

You don't need to reference issue (it's already in commit message) here, better comment like "// Removing remaining '->' from overloaded operator call"

[mikecrowe]

In D145730#4182949, @PiotrZSL wrote:

Also consider reducing commit message, instead just copying issue description.
Simple description about issue would be sufficient.

TBH, I wasn't expecting this change to be accepted. I was merely hoping that it would cause someone to give me a hint as to how to fix it properly. That's why I included so much information in the commit message. I will shorten the commit message.

No functional issues, so LGTM.

Thanks for the review.

[PiotrZSL]

Code is correct, simply this check uses more manual string manipulation.
In theory there could be some more issues if operator -> wouldn't return pointer, but a class with additional operator ->, but that's legacy independent issue.

[mikecrowe]

Fix comment in code as suggested by PiotrZSL. Improve lit check tests.

[mikecrowe]

Thanks for the speedy re-review. I don't have commit permission, so please can you land this if you are happy to do so?

[mikecrowe]

Thanks, and sorry about the erroneous [PATCH] in the summary. :(

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

if (Op->getOpcode() == UO_AddrOf) {
       return std::string(tooling::fixit::getText(
           *Op->getSubExpr()->IgnoreParens(), *Result.Context));
     }
   }
   StringRef Text = tooling::fixit::getText(ExprNode, *Result.Context);
 
   if (Text.empty())
     return std::string();
+
+  // Remove remaining '->' from overloaded operator call
+  Text.consume_back("->");
+
   // Add leading '*'.
   if (needParensAfterUnaryOperator(ExprNode)) {
     return (llvm::Twine("*(") + Text + ")").str();
   }
   return (llvm::Twine("*") + Text).str();
 }
 
 AST_MATCHER(MaterializeTemporaryExpr, isBoundToLValue) {

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

 // RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- -isystem %clang_tidy_headers
 #include <string>
 
+template <typename T>
+struct iterator {
+  T *operator->();
+  T &operator*();
+};
+
 namespace llvm {
 struct StringRef {
   StringRef(const char *p);
   StringRef(const std::string &);
 };
 }
 
 // Tests for std::string.
void m1(std::string&&) {
   m1p1 = m1;
   m1p1(s.c_str());
 
   using m1tp = void (*)(std::string &&);
   m1tp m1p2 = m1;
   m1p2(s.c_str());  
 }
 
+// Test for overloaded operator->
+void it(iterator<std::string> i)
+{
+  std::string tmp;
+  tmp = i->c_str();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}tmp = *i;{{$}}
+
+  // An unlikely situation and the outcome is not ideal, but at least the fix doesn't generate broken code.
+  tmp = i.operator->()->c_str();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}tmp = *i.operator->();{{$}}
+
+  // The fix contains an unnecessary set of parentheses, but these have no effect.
+  iterator<std::string> *pi = &i;
+  tmp = (*pi)->c_str();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}tmp = *(*pi);{{$}}
+
+  // An unlikely situation, but at least the fix doesn't generate broken code.
+  tmp = pi->operator->()->c_str();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant call to 'c_str' [readability-redundant-string-cstr]
+  // CHECK-FIXES: {{^  }}tmp = *pi->operator->();{{$}}
+}
+
 namespace PR45286 {
 struct Foo {
   void func(const std::string &) {}
   void func2(std::string &&) {}
 };
 
 void bar() {
   std::string Str{"aaa"};