This is an archive of the discontinued LLVM Phabricator instance.

[gtest] Fix printing of StringRef and SmallString in assert messages.
ClosedPublic

Authored by sammccall on Aug 21 2019, 2:57 AM.

Details

Summary

These are detected by gtest as containers, and so previously printed as e.g.

{ '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) },

gtest itself overloads PrintTo for std::string and friends, we use the same mechanism.

Event Timeline

sammccall created this revision.Aug 21 2019, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 2:57 AM
sammccall updated this revision to Diff 216361.Aug 21 2019, 3:00 AM

revert extraneous include

jhenderson added inline comments.
llvm/unittests/ADT/SmallStringTest.cpp
206

as a string

209

Do you really need the R"" syntax for the string here? It seems a bit unnecessary when "foo" should suffice.

llvm/unittests/ADT/StringRefTest.cpp
1059

as a string

labath accepted this revision.Aug 21 2019, 3:58 AM

Thanks for the quick turnaround. This looks good to me. If there's a way to use the regular std::string printer without going through the internal API, then I think it would be better to do that, but what you have here doesn't look too bad either.

llvm/unittests/ADT/SmallStringTest.cpp
209

I believe the string will come out as "foo", which would be "\"foo\"" in non-raw form.

llvm/utils/unittest/googletest/include/gtest/internal/custom/gtest-printers.h
54

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)?

This revision is now accepted and ready to land.Aug 21 2019, 3:58 AM
sammccall updated this revision to Diff 216372.Aug 21 2019, 4:34 AM
sammccall marked 4 inline comments as done.

review comments

sammccall added inline comments.Aug 21 2019, 4:34 AM
llvm/unittests/ADT/SmallStringTest.cpp
209

Right: EXPECT_EQ("\"foo\"", ...) doesn't seem any more readable to me.

llvm/utils/unittest/googletest/include/gtest/internal/custom/gtest-printers.h
54

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)

This revision was automatically updated to reflect the committed changes.