[PATCH] D145730: [clang-tidy] readability-redundant-string-cstr for smart pointer #576705

[Mike Crowe via Phabricator via cfe-commits]

mikecrowe updated this revision to Diff 504108.
mikecrowe marked an inline comment as done.
mikecrowe added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145730/new/

https://reviews.llvm.org/D145730

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


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
@@ -1,6 +1,12 @@
 // RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- 
-isystem %clang_tidy_headers
 #include 
 
+template 
+struct iterator {
+  T *operator->();
+  T *();
+};
+
 namespace llvm {
 struct StringRef {
   StringRef(const char *p);
@@ -202,6 +208,31 @@
   m1p2(s.c_str());  
 }
 
+// Test for overloaded operator->
+void it(iterator 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 *pi = 
+  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 &) {}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -52,6 +52,10 @@
 
   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();


Index: clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
@@ -1,6 +1,12 @@
 // RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- -isystem %clang_tidy_headers
 #include 
 
+template 
+struct iterator {
+  T *operator->();
+  T *();
+};
+
 namespace llvm {
 struct StringRef {
   StringRef(const char *p);
@@ -202,6 +208,31 @@
   m1p2(s.c_str());  
 }
 
+// Test for overloaded operator->
+void it(iterator 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 *pi = 
+  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 &) {}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ 

[PATCH] D145730: [clang-tidy] readability-redundant-string-cstr for smart pointer #576705

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145730/new/

https://reviews.llvm.org/D145730

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145730: [clang-tidy] readability-redundant-string-cstr for smart pointer #576705

[Mike Crowe via Phabricator via cfe-commits]

mikecrowe added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145730/new/

https://reviews.llvm.org/D145730

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145730: [clang-tidy] readability-redundant-string-cstr for smart pointer #576705

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL accepted this revision.
PiotrZSL added a comment.
This revision is now accepted and ready to land.

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

No functional issues, so LGTM.




Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:56
+
+  // https://github.com/llvm/llvm-project/issues/56705
+  Text.consume_back("->");

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145730/new/

https://reviews.llvm.org/D145730

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145730: [clang-tidy] readability-redundant-string-cstr for smart pointer #576705

[Mike Crowe via Phabricator via cfe-commits]

mikecrowe created this revision.
mikecrowe added a reviewer: njames93.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a project: All.
mikecrowe requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The readability-redundant-string-cstr check incorrectly replaces calls
to c_str() that involve an overloaded operator->.

Running `clang-tidy --checks="-*,readability-redundant-string-cstr"` on:

#include 
 #include 

void it(std::vector::const_iterator i)
 {

  std::string tmp;
  tmp = i->c_str();

}

yields:

/home/mac/git/llvm-project/build/../bug.cpp:7:9: warning: redundant call to 
'c_str' [readability-redundant-string-cstr]

  tmp = i->c_str();
^~
*i->

which means that the code is "fixed" to incorrectly say:

  tmp = *i->;

rather than the expected:

  tmp = *i;

This appears to be due to the overloaded `operator->` meaning that
RedundantStringCStrCheck.cpp#L53 ends up with Text containing "i->"
rather than just the expected "i".

Add test case for this and fix it in a somewhat nasty manner in the
absence of something better.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145730

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


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
@@ -1,6 +1,11 @@
 // RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- 
-isystem %clang_tidy_headers
 #include 
 
+template 
+struct iterator {
+  T *operator->();
+};
+
 namespace llvm {
 struct StringRef {
   StringRef(const char *p);
@@ -202,6 +207,15 @@
   m1p2(s.c_str());  
 }
 
+// Test for iterator
+void it(iterator 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;{{$}}
+}
+
 namespace PR45286 {
 struct Foo {
   void func(const std::string &) {}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -52,6 +52,10 @@
 
   if (Text.empty())
 return std::string();
+
+  // https://github.com/llvm/llvm-project/issues/56705
+  Text.consume_back("->");
+
   // Add leading '*'.
   if (needParensAfterUnaryOperator(ExprNode)) {
 return (llvm::Twine("*(") + Text + ")").str();


Index: clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/redundant-string-cstr.cpp
@@ -1,6 +1,11 @@
 // RUN: %check_clang_tidy %s readability-redundant-string-cstr %t -- -- -isystem %clang_tidy_headers
 #include 
 
+template 
+struct iterator {
+  T *operator->();
+};
+
 namespace llvm {
 struct StringRef {
   StringRef(const char *p);
@@ -202,6 +207,15 @@
   m1p2(s.c_str());  
 }
 
+// Test for iterator
+void it(iterator 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;{{$}}
+}
+
 namespace PR45286 {
 struct Foo {
   void func(const std::string &) {}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
@@ -52,6 +52,10 @@
 
   if (Text.empty())
 return std::string();
+
+  // https://github.com/llvm/llvm-project/issues/56705
+  Text.consume_back("->");
+
   // Add leading '*'.
   if (needParensAfterUnaryOperator(ExprNode)) {
 return (llvm::Twine("*(") + Text + ")").str();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits