Page MenuHomePhabricator

[clang-tidy] Fix RedundantStringCStrCheck with r values
ClosedPublic

Authored by njames93 on Wed, Jul 29, 3:06 AM.

Details

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.

Diff Detail

Event Timeline

njames93 created this revision.Wed, Jul 29, 3:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 29, 3:06 AM
njames93 requested review of this revision.Wed, Jul 29, 3:06 AM
gribozavr2 accepted this revision.Wed, Jul 29, 4:32 AM

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.

This revision is now accepted and ready to land.Wed, Jul 29, 4:32 AM
This revision was automatically updated to reflect the committed changes.

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