[PATCH] D66520: [gtest] Fix printing of StringRef and SmallString in assert messages.

Sam McCall via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 21 04:34:45 PDT 2019


sammccall added inline comments.


================
Comment at: llvm/unittests/ADT/SmallStringTest.cpp:209
+TEST_F(SmallStringTest, GTestPrinter) {
+  EXPECT_EQ(R"("foo")", ::testing::PrintToString(SmallString<1>("foo")));
+  const SmallVectorImpl<char> &ErasedSmallString = SmallString<1>("foo");
----------------
labath wrote:
> jhenderson wrote:
> > Do you really need the R"" syntax for the string here? It seems a bit unnecessary when "foo" should suffice.
> I believe the string will come out as `"foo"`, which would be `"\"foo\""` in non-raw form.
Right: `EXPECT_EQ("\"foo\"", ...)` doesn't seem any more readable to me.


================
Comment at: llvm/utils/unittest/googletest/include/gtest/internal/custom/gtest-printers.h:54
+inline void PrintTo(llvm::StringRef S, std::ostream *OS) {
+  ::testing::internal::PrintStringTo(S.str(), OS);
+}
----------------
labath wrote:
> Could these delegate to the "public" `StringTo` function instead of going for the internal API (`PrintTo(S.str(), OS)`, or whatever is the right ADL magic for calling that)?
Done via `PrintToString`. It's an extra copy, but I don't think it matters here.

We can't call the `std::string` overload of `PrintTo` via ADL as it's not in namespace std, gtest relies on regular lookup from a callsite inside `::testing::internal` for that one.

(I think `::testing::internal::PrintTo` might actually be a badly-named *public* API: it's documented as such in `gtest-printers.h` alongside `::testing::PrintToString`. But no reason to worry about the ambiguity)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66520





More information about the llvm-commits mailing list