Add StringView ctor to Matcher<std::string> for lifetime-safe Property() / Field() use#4968
Draft
Cutuy wants to merge 1 commit into
Draft
Add StringView ctor to Matcher<std::string> for lifetime-safe Property() / Field() use#4968Cutuy wants to merge 1 commit into
Cutuy wants to merge 1 commit into
Conversation
…ring&>
Currently, Matcher<StringView>::Matcher(StringView s) and
Matcher<const StringView&>::Matcher(StringView s) exist and explicitly copy
into a std::string so the matcher owns its data:
Matcher<internal::StringView>::Matcher(internal::StringView s) {
*this = Eq(std::string(s));
}
But the symmetric Matcher<const std::string&>::Matcher(StringView) and
Matcher<std::string>::Matcher(StringView) do not exist. That asymmetric gap
silently miscompiles when callers pass a string_view temporary into matchers
whose target type is std::string -- e.g.
Property(&Proto::string_field, std::format("{},{}", subkey, topic))
Property() builds Matcher<const std::string&> from the value. Since no
StringView constructor exists for Matcher<const std::string&>, the
string_view falls through to MatcherCastImpl, which builds a polymorphic
Eq(string_view). EqMatcher<string_view> stores the view by value, and the
matcher outlives the calling expression -- the underlying buffer (the
std::format temporary) is destroyed and the matcher dangles.
Allocator reuse in optimized builds reliably exposes this: subsequent
std::format calls reuse the same heap slot, and all stored matchers end up
pointing into the same recycled buffer with the last-written contents.
This patch adds the symmetric constructors that mirror the existing
Matcher<StringView>(StringView) impl: copy into a std::string via
Eq(std::string(s)) so the matcher owns its data.
Also adds:
- StringMatcherTest.CanBeImplicitlyConstructedFromStringView
- StringMatcherTest.StringViewCtorIsLifetimeSafe (regression test that
exercises the matcher after the source buffer is destroyed/mutated)
Signed-off-by: Jason Cui <jcui@nuro.ai>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
/recheck |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
Matcher<std::string>::Matcher(StringView)andMatcher<const std::string&>::Matcher(StringView), mirroring the existingMatcher<StringView>::Matcher(StringView)implementation. The new constructors copy into astd::stringviaEq(std::string(s))so the matcher owns its data.Motivation
Today,
Matcher<StringView>has an explicit constructor that copies into astd::string:Matcher<internal::StringView>::Matcher(internal::StringView s) { *this = Eq(std::string(s)); }…but the symmetric
Matcher<std::string>/Matcher<const std::string&>do not. That asymmetric gap silently miscompiles when callers pass astring_viewtemporary into a matcher whose target type isstd::string— most commonly viaProperty/Field:PropertybuildsMatcher<const std::string&>from the value. With noStringViewctor available for that target, thestring_viewfalls through toMatcherCastImpland ends up wrapped by polymorphicEq(string_view)→EqMatcher<string_view>stores the view by value. The matcher persists until the test fixture is destroyed; thestd::formattemporary it points to is destroyed at the end of theEXPECT_CALLstatement. The matcher dangles.Allocator reuse in optimized builds reliably exposes this: subsequent
std::formatcalls in the same scope reuse the same heap slot, so multiple stored matchers all end up pointing into the same recycled buffer holding the last-written contents. We hit it in production code where sixEXPECT_CALLs withProperty(&Proto::sub_key, std::format(...))collapsed to all matching the same garbled string in-c opt(but passing in fastbuild).Change
Matcher(StringView)constructor onMatcher<std::string>andMatcher<const std::string&>, guarded byGTEST_INTERNAL_HAS_STRING_VIEW.gtest-matchers.ccthat delegate toEq(std::string(s)), exactly mirroring the existingMatcher<StringView>(StringView)impl.The change is purely additive — there is no new behavior for code that doesn't currently dangle, and the new path makes a previously-undefined-behavior case well-defined.
Tests
StringMatcherTest.CanBeImplicitlyConstructedFromStringView— symmetric counterpart of the existingStringViewMatcherTest.CanBeImplicitlyConstructedFromString.StringMatcherTest.StringViewCtorIsLifetimeSafe— regression test that constructs the matcher from aStringViewof astd::stringbuffer, then mutates and destroys the buffer before invokingMatches(). Without this patch the matcher would hold a dangling view; with the patch it holds an owned copy.Both pass under cmake locally (
gmock-matchers-comparisons_test).Test plan
StringViewMatcherTest.*andStringMatcherTest.*still passgmock-matchers-comparisons_testbuilds cleanly🤖 Generated with Claude Code