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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74033



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

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

[Karasev Nikita via Phabricator via cfe-commits]

f00kat added a comment.

Yes, commit please. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74033



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

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

[Aaron Ballman via Phabricator via cfe-commits]

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74033



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

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

[Karasev Nikita via Phabricator via cfe-commits]

f00kat updated this revision to Diff 243833.
f00kat added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74033

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
@@ -205,3 +205,18 @@
 void invalid(const NotAString ) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string.
+void m1(std::string&&) {
+  std::string s;
+
+  m1(s.c_str());
+
+  void (*m1p1)(std::string&&);
+  m1p1 = m1;
+  m1p1(s.c_str());
+
+  using m1tp = void (*)(std::string &&);
+  m1tp m1p2 = m1;
+  m1p2(s.c_str());  
+}
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
@@ -61,6 +61,55 @@
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get CallExpr in which CxxConstructExpr is called.
+static const clang::CallExpr *
+tryGetCallExprAncestorForCxxConstructExpr(const Expr *TheExpr,
+  ASTContext ) {
+  // We skip nodes such as CXXBindTemporaryExpr, MaterializeTemporaryExpr.
+  for (ast_type_traits::DynTypedNode DynParent : Context.getParents(*TheExpr)) 
{
+if (const auto *Parent = DynParent.get()) {
+  if (const auto *TheCallExpr = dyn_cast(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 ) {
+  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 *TheCallExprFuncProto =
+TheCallExpr->getCallee()
+->getType()
+->getPointeeType()
+->getAs()) {
+  if (TheCallExprFuncProto->getParamType(i)->isRValueReferenceType())
+return true;
+}
+  }
+}
+  }
+
+  return false;
+}
+
+AST_MATCHER(CXXConstructExpr,
+matchedParamDeclOfAncestorCallExprHasRValueRefType) {
+  return checkParamDeclOfAncestorCallExprHasRValueRefType(
+  , Finder->getASTContext());
+}
+
 } // end namespace
 
 void RedundantStringCStrCheck::registerMatchers(
@@ -95,9 +144,13 @@
   .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())),
+  this);
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(


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
@@ -205,3 +205,18 @@
 void invalid(const NotAString ) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string.
+void m1(std::string&&) {
+  std::string s;
+
+  m1(s.c_str());
+
+  void (*m1p1)(std::string&&);
+  m1p1 = m1;
+  m1p1(s.c_str());
+
+  using m1tp = void (*)(std::string &&);
+  m1tp m1p2 = m1;
+  m1p2(s.c_str());  
+}
Index: clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp
===
--- 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:64
 
+// Trying to get CallExpr in which CxxConstructExpr is called
+const clang::CallExpr *

Missing a full stop at the end of the comment.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:66
+const clang::CallExpr *
+tryGetCallExprAncestorForCxxConstructExpr(const Expr *TheExpr,
+ ASTContext ) {

This function and the one below can be `static`.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:83
+
+// Check that ParamDecl of CallExprDecl has rvalue type
+bool checkParamDeclOfAncestorCallExprHasRValueRefType(

Missing full stop at the end of the comment.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:93
+if (const auto *TheCallExprDecl =
+dyn_cast(TheCallExpr->getCalleeDecl())) {
+  if (TheCallExprDecl->getParamDecl(i)

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.)



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantStringCStrCheck.cpp:147
+  // If CxxConstructExpr is the part of some CallExpr we need to
+  // check that matched ParamDecl of the ancestor CallExpr is not rvalue
+  Finder->addMatcher(

Missing a full stop at the end of the comment.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-cstr.cpp:209-210
+
+// Test for rvalue std::string
+
+void m1(std::string&& s) {

Missing full stop, can also remove the trailing newline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74033



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

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

[Karasev Nikita via Phabricator via cfe-commits]

f00kat created this revision.
f00kat added reviewers: aaron.ballman, alexfh.
f00kat added projects: clang-tools-extra, clang.
Herald added subscribers: cfe-commits, xazax.hun.

  #include 
  
  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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74033

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
@@ -205,3 +205,9 @@
 void invalid(const NotAString ) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string
+
+void m1(std::string&& s) {
+  m1(s.c_str());
+}
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
@@ -61,6 +61,54 @@
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get CallExpr in which CxxConstructExpr is called
+const clang::CallExpr *
+tryGetCallExprAncestorForCxxConstructExpr(const Expr *TheExpr,
+ ASTContext ) {
+  // We skip nodes such as CXXBindTemporaryExpr, MaterializeTemporaryExpr
+  for (ast_type_traits::DynTypedNode DynParent : Context.getParents(*TheExpr)) 
{
+if (const auto *Parent = DynParent.get()) {
+  if (const auto *TheCallExpr = dyn_cast(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 ) {
+  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(TheCallExpr->getCalleeDecl())) {
+  if (TheCallExprDecl->getParamDecl(i)
+  ->getType()
+  ->isRValueReferenceType())
+return true;
+}
+  }
+}
+  }
+
+  return false;
+}
+
+AST_MATCHER(CXXConstructExpr,
+matchedParamDeclOfAncestorCallExprHasRValueRefType) {
+  return checkParamDeclOfAncestorCallExprHasRValueRefType(
+  , Finder->getASTContext());
+}
+
 } // end namespace
 
 void RedundantStringCStrCheck::registerMatchers(
@@ -95,9 +143,13 @@
   .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())),
+  this);
 
   // Detect: 's == str.c_str()'  ->  's == str'
   Finder->addMatcher(


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
@@ -205,3 +205,9 @@
 void invalid(const NotAString ) {
   dummy(s.c_str());
 }
+
+// Test for rvalue std::string
+
+void m1(std::string&& s) {
+  m1(s.c_str());
+}
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
@@ -61,6 +61,54 @@
   return (llvm::Twine("*") + Text).str();
 }
 
+// Trying to get CallExpr in which