mikecrowe created this revision. mikecrowe added a reviewer: njames93. mikecrowe added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, xazax.hun. Herald added a project: All. mikecrowe requested review of this revision. Herald added a subscriber: cfe-commits.
std::format (C++20) and std::print (C++23) are perfectly happy to accept std::string parameters. Converting them to C-style strings by calling c_str() is unnecessary and may cause extra walking of the string to determine its length. It's straightforward to teach readability-redundant-string-cstr to identify the first such unnecessary call to c_str() in the parameter list, but I was unable to come up with a way that worked for all the parameters. I was unable to come up with a way to make libstdc++'s format_string first parameter to std::format and std::print work in the redundant-string-cstr.cpp lit check. Using const char * is sufficient though since that isn't the argument we're interested in. I was able to successfully run the readability-redundant-string-cstr check on a real source file that compiled successfully with what is destined to become GCC 13, and saw the expected: --8<-- /home/mac/git/llvm-project/build/bin/clang-tidy -checks=-*,readability-redundant-string-cstr format.cpp -- --gcc-toolchain=/home/mac/gcc-git 6 warnings generated. /home/mac/src/random-hacks/std-format/format.cpp:13:39: warning: redundant call to 'c_str' [readability-redundant-string-cstr] std::puts(std::format("Hello {}", get().c_str()).c_str()); ^~~~~~~~~~~~~ get() Suppressed 5 warnings (5 with check filters). -->8-- Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D143342 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 @@ -291,3 +291,59 @@ Foo.func2((Str.c_str())); } } // namespace PR45286 + +namespace std { + template<typename ...Args> + void print(const char *, Args &&...); + template<typename ...Args> + std::string format(const char *, Args &&...); +} + +namespace notstd { + template<typename ...Args> + void print(const char *, Args &&...); + template<typename ...Args> + std::string format(const char *, Args &&...); +} + +void std_print(const std::string &s1, const std::string &s2, const std::string &s3) { + std::print("One:{}\n", s1.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}std::print("One:{}\n", s1); + + // Ideally we'd fix both the second and fourth parameters here, but that doesn't work. + std::print("One:{} Two:{} Three:{}\n", s1.c_str(), s2, s3.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}std::print("One:{} Two:{} Three:{}\n", s1, s2, s3.c_str()); +} + +// There's no c_str() call here, so it shouldn't be touched. +void std_print_no_cstr(const std::string &s1, const std::string &s2) { + std::print("One: {}, Two: {}\n", s1, s2); +} + +// This isn't std::print, so it shouldn't be fixed. +void not_std_print(const std::string &s1) { + notstd::print("One: {}\n", s1.c_str()); +} + +void std_format(const std::string &s1, const std::string &s2, const std::string &s3) { + auto r1 = std::format("One:{}\n", s1.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}auto r1 = std::format("One:{}\n", s1); + + // Ideally we'd fix both the second and fourth parameters here, but that doesn't work. + auto r2 = std::format("One:{} Two:{} Three:{}\n", s1.c_str(), s2, s3.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:53: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}auto r2 = std::format("One:{} Two:{} Three:{}\n", s1, s2, s3.c_str()); +} + +// There's are c_str() calls here, so it shouldn't be touched. +std::string std_format_no_cstr(const std::string &s1, const std::string &s2) { + return std::format("One: {}, Two: {}\n", s1, s2); +} + +// This is not std::format, so it shouldn't be fixed. +std::string not_std_format(const std::string &s1) { + return notstd::format("One: {}\n", s1.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 @@ -178,6 +178,14 @@ // directly. hasArgument(0, StringCStrCallExpr))), this); + + Finder->addMatcher( + traverse(TK_AsIs, + callExpr(anyOf(callee(functionDecl(hasName("::std::print"))), + callee(functionDecl(hasName("::std::format")))), + hasAnyArgument(materializeTemporaryExpr( + has(StringCStrCallExpr))))), + this); } void RedundantStringCStrCheck::check(const MatchFinder::MatchResult &Result) {
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 @@ -291,3 +291,59 @@ Foo.func2((Str.c_str())); } } // namespace PR45286 + +namespace std { + template<typename ...Args> + void print(const char *, Args &&...); + template<typename ...Args> + std::string format(const char *, Args &&...); +} + +namespace notstd { + template<typename ...Args> + void print(const char *, Args &&...); + template<typename ...Args> + std::string format(const char *, Args &&...); +} + +void std_print(const std::string &s1, const std::string &s2, const std::string &s3) { + std::print("One:{}\n", s1.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}std::print("One:{}\n", s1); + + // Ideally we'd fix both the second and fourth parameters here, but that doesn't work. + std::print("One:{} Two:{} Three:{}\n", s1.c_str(), s2, s3.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}std::print("One:{} Two:{} Three:{}\n", s1, s2, s3.c_str()); +} + +// There's no c_str() call here, so it shouldn't be touched. +void std_print_no_cstr(const std::string &s1, const std::string &s2) { + std::print("One: {}, Two: {}\n", s1, s2); +} + +// This isn't std::print, so it shouldn't be fixed. +void not_std_print(const std::string &s1) { + notstd::print("One: {}\n", s1.c_str()); +} + +void std_format(const std::string &s1, const std::string &s2, const std::string &s3) { + auto r1 = std::format("One:{}\n", s1.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}auto r1 = std::format("One:{}\n", s1); + + // Ideally we'd fix both the second and fourth parameters here, but that doesn't work. + auto r2 = std::format("One:{} Two:{} Three:{}\n", s1.c_str(), s2, s3.c_str()); + // CHECK-MESSAGES: :[[@LINE-1]]:53: warning: redundant call to 'c_str' [readability-redundant-string-cstr] + // CHECK-FIXES: {{^ }}auto r2 = std::format("One:{} Two:{} Three:{}\n", s1, s2, s3.c_str()); +} + +// There's are c_str() calls here, so it shouldn't be touched. +std::string std_format_no_cstr(const std::string &s1, const std::string &s2) { + return std::format("One: {}, Two: {}\n", s1, s2); +} + +// This is not std::format, so it shouldn't be fixed. +std::string not_std_format(const std::string &s1) { + return notstd::format("One: {}\n", s1.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 @@ -178,6 +178,14 @@ // directly. hasArgument(0, StringCStrCallExpr))), this); + + Finder->addMatcher( + traverse(TK_AsIs, + callExpr(anyOf(callee(functionDecl(hasName("::std::print"))), + callee(functionDecl(hasName("::std::format")))), + hasAnyArgument(materializeTemporaryExpr( + has(StringCStrCallExpr))))), + this); } void RedundantStringCStrCheck::check(const MatchFinder::MatchResult &Result) {
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits