overload PrintTo for std::type_info and std::type_index

[N-Dekker]

Included the string returned by their name() member function with the output of PrintTo.

Typical use case:

std::unique_ptr<AbstractProduct> product = FactoryMethod();
// Assert that the product is of type X:
ASSERT_EQ(std::type_index{typeid(*product)},
          std::type_index{typeid(ProductX)});

Possible output in case of a test assert failure, now including the names of the compared type indices:

error: Expected equality of these values:
std::type_index(typeid(*product))
Which is: 8-byte object <D0-65 54-8C F6-7F 00-00> ("class ProductY")
std::type_index(typeid(ProductX))
Which is: 8-byte object <40-64 54-8C F6-7F 00-00> ("class ProductX")

With help from Krystian Kuzniarek (@kuzkry).

[kuzkry]

Hi! I have two findings here:

1. older std::type_info doesn't have its printer because it falls back to the byte printer. Do you plan on adding it too?
2. your code should probably depend on GTEST_HAS_RTTI.

[N-Dekker]

@kuzkry Thanks for your comment, Krystian.

1. older std::type_info doesn't have its printer because it falls back to the byte printer. Do you plan on adding it too?

Thanks for the suggestion. I think I'll amend this pull request tomorrow, to add an std::type_info overload as well!

2. your code should probably depend on GTEST_HAS_RTTI.

Are you sure it should? I mean, would it harm to query the name of a type when GTEST_HAS_RTTI is false? The tests at https://travis-ci.org/github/google/googletest/jobs/751504899 (clang C++ NO_RTTI=ON ) seem to have passed. If name() only returns the compile-time type, it may still be helpful for some use cases. If name() just returns an empty string, it may not really harm either. What do you think?

[N-Dekker]

@kuzkry Can you please check if I have addressed both of your findings correctly by my force-pushes?

[kuzkry]

@kuzkry Thanks for your comment, Krystian.

Heh, I see you took your time to find my actual name, nice :D

1. older std::type_info doesn't have its printer because it falls back to the byte printer. Do you plan on adding it too?

Thanks for the suggestion. I think I'll amend this pull request tomorrow, to add an std::type_info overload as well!

Thanks too :) That looks good to me.

2. your code should probably depend on GTEST_HAS_RTTI.

Are you sure it should? I mean, would it harm to query the name of a type when GTEST_HAS_RTTI is false? The tests at https://travis-ci.org/github/google/googletest/jobs/751504899 (clang C++ NO_RTTI=ON ) seem to have passed. If name() only returns the compile-time type, it may still be helpful for some use cases. If name() just returns an empty string, it may not really harm either. What do you think?

Good point, indeed. Therefore, it's not like it should, but it could :) I think it does no harm really and you're right, we don't need it. If we keep it without GTEST_HAS_RTTI, it's one less #ifdef anyway. Maybe someone's gonna say that the slight advantage of using GTEST_HAS_RTTI is that code is more "greppable" or maybe that it's inconsistent with some logic / expectations of theirs on that switch (but it's merely a header switch anyway and some funny combinations like enabled RTTI in one's compiler and GTEST_HAS_RTTI=OFF are possible). I'm indifferent tbh but luckily this PR is gonna be looked into by Google maintainers :D

@kuzkry Can you please check if I have addressed both of your findings correctly by my force-pushes?

Sure. I'm gonna reply in the thread of your inline comment.

Last but not least, I've just come up with this one. Did you know this?

There is no guarantee that the same std::type_info instance will be referred to by all evaluations of the typeid expression on the same type, although std::type_info::hash_code of those type_info objects would be identical, as would be their std::type_index.
~ https://en.cppreference.com/w/cpp/language/typeid

So our equality matchers may fail on this:

const std::type_info& ti1 = typeid(A);
const std::type_info& ti2 = typeid(A);
 
EXPECT_EQ(&ti1, &ti2); // not guaranteed

But maybe it's too much for the testing framework and it shouldn't take care of it.

[kuzkry]

This is the point where I can only say I like it and wish you good luck with getting it approved by maintainers :)

[N-Dekker]

For the record, it appears that the following compiles well, even with -fno-rtti (which I assume to correspond with GTEST_HAS_RTTI = 0):

#include <typeinfo>
#include <typeindex>

std::type_index f(std::type_index index, std::type_info info)
{
    if ((index == info) || (index.name() == info.name()))
    {
        return index;
    }
    throw index;
}

Tested on GCC 10.2 and Clang 11.0.0 at https://godbolt.org/z/jsvr1s

Whereas a simple typeid(int) fails at https://godbolt.org/z/vvrff8 (having -fno-rtti), saying:

error: cannot use 'typeid' with '-fno-rtti'

Visual C++ does seem to support typeid(int) even when run-time type information is disabled (/GR-), but it may produce a warning on the following lines of code:

const Base& base = Derived{};
const auto& base_typeid = typeid(base);

msvc v19.28 says at https://godbolt.org/z/Ps85nf

warning C4541: 'typeid' used on polymorphic type 'Base' with /GR-; unpredictable behavior may result

[N-Dekker]

This is the point where I can only say I like it and wish you good luck with getting it approved by maintainers :)

@kuzkry Thank you very much for your approval, Krystian!

[N-Dekker]

There is no guarantee that the same std::type_info instance will be referred to by all evaluations of the typeid expression on the same type, although std::type_info::hash_code of those type_info objects would be identical, as would be their std::type_index.
~ https://en.cppreference.com/w/cpp/language/typeid

So our equality matchers may fail on this:

const std::type_info& ti1 = typeid(A);
const std::type_info& ti2 = typeid(A);
 
EXPECT_EQ(&ti1, &ti2); // not guaranteed

@kuzkry Indeed! Fortunately, EXPECT_EQ(ti1, ti2) should be just fine. Right? (Guaranteed successfully passed.) Using std::type_info::operator==(const type_info&).

[kuzkry]

There is no guarantee that the same std::type_info instance will be referred to by all evaluations of the typeid expression on the same type, although std::type_info::hash_code of those type_info objects would be identical, as would be their std::type_index.
~ https://en.cppreference.com/w/cpp/language/typeid

So our equality matchers may fail on this:

const std::type_info& ti1 = typeid(A);
const std::type_info& ti2 = typeid(A);
 
EXPECT_EQ(&ti1, &ti2); // not guaranteed

@kuzkry Indeed! Fortunately, EXPECT_EQ(ti1, ti2) should be just fine. Right? (Guaranteed successfully passed.) Using std::type_info::operator==(const type_info&).

Yeah, no problem with it then. It becomes problematic when addresses are compared.

[kuzkry]

Sorry to bother you, but I've found something about GTEST_HAS_RTTI here:

googletest/googletest/include/gtest/internal/gtest-port.h

Line 529 in d128fc8

// It's this header's responsibility to #include <typeinfo> when RTTI

gtest-port.h includes <typeinfo> there conditionally (no other file in GoogleTest includes this btw.) so I think that's the place where you should move your <typeindex> inclusion and actually hide your printers under #ifdef GTEST_HAS_RTTI. Upon closer inspection, I think this fits better because everything related to RTTI relies on this switch and it's strictly followed rule.

[kuzkry]

I would like just to unapprove it but GitHub forces me to request changes.

[N-Dekker]

gtest-port.h includes <typeinfo> there conditionally (no other file in GoogleTest includes this btw.) so I think that's the place where you should move your <typeindex> inclusion and actually hide your printers under #ifdef GTEST_HAS_RTTI. Upon closer inspection, I think this fits better because everything related to RTTI relies on this switch and it's strictly followed rule.

@kuzkry Thanks for having another look at this pull request. Can you please elaborate a little bit more? Specifically, could those two #include directives in "gtest-printers.h" possibly cause any technical problems?

googletest/googletest/include/gtest/gtest-printers.h

Lines 109 to 110 in ae50871

	#include <typeindex>
	#include <typeinfo>

After having tried -fno-rtti on both Clang and GCC, as well as /GR- (disable run-time type info) on MSVC, I concluded that it appears OK to #include those headers, and use the Standard Library types std::type_info and std::type_index, even when RTTI is disabled. (Only the use of the typeid language feature appeared troublesome, when RTTI is disabled.) Do you agree?

[kuzkry]

Hey @N-Dekker, I'm sorry for not answering that earlier :( It slipped through my list.

I think you're right and what you did is fine 👍 I just meant we should probably try to conform to existing style and unify it with how RTTI is generally handled in GoogleTest.

I didn't want to go into details but gtest-port.h is quite special.

// This file is fundamental to Google Test. All other Google Test source
// files are expected to #include this. Therefore, it cannot #include
// any other Google Test header.
~ from line 37

gtest-port.h exists so that GoogleTest can keep platform specific dilemmas away from the rest of GoogleTest code and make it more platform agnostic. And it makes sense that support for RTTI is determined there and the rest of the code simply conforms to that, hence the comment:

// It's this header's responsibility to #include when RTTI
// is enabled.
~ from line 529

Last but not least, you can always empirically verify that all RTTI-specific classes (from standard <typeinfo> and <typeindex>) and typeid() , are indeed ALWAYS protected by GTEST_HAS_RTTI and I believe those are:

• type_info
• bad_typeid
• bad_cast
• type_index
• typeid

[N-Dekker]

@kuzkry Thanks you for your explanation, Krystian. Personally I would rather not add an extra #ifdef, if it isn't technically necessary. However, I would certainly be willing to add such an #ifdef if it would help to get this pull request accepted. But then I would like to know if this pull request makes a chance anyway, one way or the other. 🤔

[dinord]

I agree with Krystian's suggestion to guard the overloads with an ifdef:

• they are useless if RTTI isn't enabled anyway, guards are just an explicit way of pointing that out
• they'd be consistent with the rest of GoogleTest (see GetTypeName for almost identical example).

Otherwise, this PR looks good to me.

[N-Dekker]

@dinord @kuzkry FYI The force-push that I just did is only the result of a git rebase master, just to get up-to-date again. With my next force-push I hope to address the RTTI issue that you raised. Thanks so far!

[N-Dekker]

@dinord @kuzkry Update: Addressed the RTTI issue with my latest force-push, I hope it's OK to you!

[N-Dekker]

@dinord Hi Dino! Thanks you for looking at this pull request of mine! I see you added a label, last week, "imported-into-CL", I'm interested to hear from you what that means! Anyway, I really hope this PR will make it onto the master branch 😃 What do you think?

[kuzkry]

My guess is that it's a maintainers' marker saying your PR is actually being reviewed in the context of true GoogleTest. Because actually this repository is a mirror and the original one is stored on Google servers. It probably runs some superior continuous integration that checks if that breaks code at Google. I guess typo fixes and easy stuff do not require expensive validations to be done so you won't see it in every PR.

Please hire me 😂

[dinord]

@N-Dekker your PR is going through our internal import process.

[N-Dekker]

@dinord Thank you very much for merging my pull request, Dino! It's the very first pull request of mine that made it onto the master branch of GoogleTest (while other attempts of mine, pull request #3191 and issue #2636, did not make it). I'm very proud to have made a contribution to GoogleTest! 😃

[dinord]

Thank you for contributing to GoogleTest, Niels!