From 5062aa991f6257d8fdccb1dfeea676bca7f7c971 Mon Sep 17 00:00:00 2001 From: Jason Cui Date: Tue, 28 Apr 2026 23:03:41 -0700 Subject: [PATCH] Add StringView ctor to Matcher and Matcher Currently, Matcher::Matcher(StringView s) and Matcher::Matcher(StringView s) exist and explicitly copy into a std::string so the matcher owns its data: Matcher::Matcher(internal::StringView s) { *this = Eq(std::string(s)); } But the symmetric Matcher::Matcher(StringView) and Matcher::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 from the value. Since no StringView constructor exists for Matcher, the string_view falls through to MatcherCastImpl, which builds a polymorphic Eq(string_view). EqMatcher 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) 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 --- .../test/gmock-matchers-comparisons_test.cc | 34 +++++++++++++++++++ googletest/include/gtest/gtest-matchers.h | 14 ++++++++ googletest/src/gtest-matchers.cc | 12 +++++++ 3 files changed, 60 insertions(+) diff --git a/googlemock/test/gmock-matchers-comparisons_test.cc b/googlemock/test/gmock-matchers-comparisons_test.cc index 3a21a9a7e..9159922fb 100644 --- a/googlemock/test/gmock-matchers-comparisons_test.cc +++ b/googlemock/test/gmock-matchers-comparisons_test.cc @@ -272,6 +272,40 @@ TEST(StringViewMatcherTest, CanBeImplicitlyConstructedFromStringView) { EXPECT_TRUE(m2.Matches("cats")); EXPECT_FALSE(m2.Matches("dogs")); } + +// Tests that a StringView object can be implicitly converted to a +// Matcher or Matcher. This is the symmetric +// counterpart of CanBeImplicitlyConstructedFromString above; without these +// constructors, a StringView falls through to a polymorphic Eq matcher that +// stores the view by value and dangles past the constructing call. +TEST(StringMatcherTest, CanBeImplicitlyConstructedFromStringView) { + Matcher m1 = internal::StringView("cats"); + EXPECT_TRUE(m1.Matches("cats")); + EXPECT_FALSE(m1.Matches("dogs")); + + Matcher m2 = internal::StringView("cats"); + EXPECT_TRUE(m2.Matches("cats")); + EXPECT_FALSE(m2.Matches("dogs")); +} + +// Tests that the matcher copies the string data and is safe to use after the +// underlying buffer the StringView was constructed from is destroyed. +TEST(StringMatcherTest, StringViewCtorIsLifetimeSafe) { + Matcher m1; + Matcher m2; + { + std::string buffer = "cats"; + m1 = internal::StringView(buffer); + m2 = internal::StringView(buffer); + // Mutate buffer and let it go out of scope to ensure the matcher does not + // alias |buffer|'s storage. + buffer = "dogs"; + } + EXPECT_TRUE(m1.Matches("cats")); + EXPECT_FALSE(m1.Matches("dogs")); + EXPECT_TRUE(m2.Matches("cats")); + EXPECT_FALSE(m2.Matches("dogs")); +} #endif // GTEST_INTERNAL_HAS_STRING_VIEW // Tests that a std::reference_wrapper object can be implicitly diff --git a/googletest/include/gtest/gtest-matchers.h b/googletest/include/gtest/gtest-matchers.h index 6d2ab14d2..a60df9aa4 100644 --- a/googletest/include/gtest/gtest-matchers.h +++ b/googletest/include/gtest/gtest-matchers.h @@ -520,6 +520,13 @@ Matcher : public internal::MatcherBase { // Allows the user to write "foo" instead of Eq("foo") sometimes. Matcher(const char* s); // NOLINT + +#if GTEST_INTERNAL_HAS_STRING_VIEW + // Allows the user to pass absl::string_views or std::string_views directly. + // Copies into a std::string so the matcher owns its data and does not alias + // a (possibly temporary) buffer in the caller. + Matcher(internal::StringView s); // NOLINT +#endif }; template <> @@ -544,6 +551,13 @@ Matcher : public internal::MatcherBase { // Allows the user to write "foo" instead of Eq("foo") sometimes. Matcher(const char* s); // NOLINT + +#if GTEST_INTERNAL_HAS_STRING_VIEW + // Allows the user to pass absl::string_views or std::string_views directly. + // Copies into a std::string so the matcher owns its data and does not alias + // a (possibly temporary) buffer in the caller. + Matcher(internal::StringView s); // NOLINT +#endif }; #if GTEST_INTERNAL_HAS_STRING_VIEW diff --git a/googletest/src/gtest-matchers.cc b/googletest/src/gtest-matchers.cc index 7e3bcc0cf..0e27f7309 100644 --- a/googletest/src/gtest-matchers.cc +++ b/googletest/src/gtest-matchers.cc @@ -93,6 +93,18 @@ Matcher::Matcher(const char* s) { Matcher::Matcher(internal::StringView s) { *this = Eq(std::string(s)); } + +// Constructs a matcher that matches a const std::string& whose value is +// equal to s. Copies into a std::string so the matcher owns its data. +Matcher::Matcher(internal::StringView s) { + *this = Eq(std::string(s)); +} + +// Constructs a matcher that matches a std::string whose value is equal to s. +// Copies into a std::string so the matcher owns its data. +Matcher::Matcher(internal::StringView s) { + *this = Eq(std::string(s)); +} #endif // GTEST_INTERNAL_HAS_STRING_VIEW } // namespace testing