Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print C++17 types (any, optional, string_view and variant) and absl::any #2438

Closed
wants to merge 4 commits into from

Conversation

kuzkry
Copy link
Contributor

@kuzkry kuzkry commented Sep 1, 2019

Resolves #2437.

Future work:
Matchers could now support wide string_views. This is something new; Abseil doesn't support this one.

@kuzkry kuzkry changed the title Print optional, string_views and variant types Print optional, string_view and variant types Sep 1, 2019
@kuzkry kuzkry changed the title Print optional, string_view and variant types Print C++17 types (any, optional, string_view and variant) and absl::any Sep 1, 2019
@kuzkry kuzkry force-pushed the cpp17-types branch 2 times, most recently from c76b1f3 to b76b791 Compare September 1, 2019 16:59
@asoffer
Copy link
Contributor

asoffer commented Sep 11, 2019

I like the idea here, but I think we need to have a bit of internal discussion first about how we want to support this.

@kuzkry
Copy link
Contributor Author

kuzkry commented Sep 12, 2019

Cool! Let me know what you lot decided, though mind I believe I've made it consistent with the current code (should you want to refactor sth).

@asoffer
Copy link
Contributor

asoffer commented Sep 23, 2019

Thanks for this PR. After internal discussion, we are going to have googletest take a hard dependency on Abseil very soon. Which means that compiling with C++17, all the types will be the standard types and have correct printing, and without C++17, the Abseil backports will have correct printing and the standard ones won't be available. So I think this PR won't be necessary. Thanks again though, for this effort as well as all the other PRs you've sent. They're super useful and very much appreciated.

@asoffer asoffer closed this Sep 23, 2019
@kuzkry
Copy link
Contributor Author

kuzkry commented Sep 23, 2019

Thank you for recognition!
But I regret your team decision as C++17 is quite popular these days. What about non-Abseil users such as myself? Will users be forced to reinvent the wheel because of this? Because having Abseil just for that is a huge no-go for my project (additional documentation and stuff...).

@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 2, 2019

@asoffer ping

@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 3, 2019

@asoffer, please note that my code uses string_views and I get bad formatting. Even my custom PrintTo() function doesn't work, check out this:

#include <string_view>

#include <gtest/gtest.h>

void PrintTo(std::string_view const s, std::ostream* const str)
{
  *str << "view";
}

TEST(FailAndPrint, MyTest)
{
  using namespace std::string_view_literals;
  EXPECT_EQ("this is"sv, "not equal"sv);
}

As a user, I'm not supposed to alter ::testing::internal and std namespaces - however in order to make my string_views work I would need to inject my function into either of these places to make ADL work.

@asoffer
Copy link
Contributor

asoffer commented Oct 3, 2019

If you're using C++17, absl::{optional, variant, any, string_view} are all the same type as the std equivalents, so unless I'm missing something this should all "just work" fine. You do not need to spell it with the absl:: namespace qualification.

If any of this doesn't work, please open up issues.

The googletest dependency on Abseil is more about using Abseil internally and should not impose any requirements on how users write tests.

@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 3, 2019

But I've already pinned a related issue :) It's #2437 and it basically tells you that not only is GTest unable to print those types, but also there are some Valgrind issues.

@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 8, 2019

Ok, @asoffer. I didn't know what you meant was the latest "Coming soon":

We are also planning to take a dependency on Abseil.

which appeared a few days ago. Yes, it will work too for std::string_view I think.

Please mind the other types!

What I wrote here:

Future work:
Matchers could now support wide string_views. This is something new; Abseil doesn't support this one.

effecitively means that this particular function will not work for std::wstring_view, std::u8string_view, std::u16string_view, std::u32string_view.
With my pull request, you could change std::string_view into std::basic_string_view<T>. Besides, it doesn't have to have the base being of the char type family to be considered as a string, as std::basic_string_view<int> is also a valid class here. Last but not least, I've already proved the users cannot provide their own PrintTo() for std types without either extending std (which is undefined behaviour btw.) or ::testing::internal (which is also against the rules).

@kuzkry
Copy link
Contributor Author

kuzkry commented Oct 22, 2019

@asoffer ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GTest is unable to print C++17 types
3 participants