Details
- Reviewers
Mordante aaron.ballman - Group Reviewers
Restricted Project - Commits
- rG16a1c851713a: [libc++] Implement P1328R1 constexpr std::type_info::operator==
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This is a really naive implementation, but I think it's sufficient.
CCing @aaron.ballman in case Clang folks want to double check that we're not doing something stupid here.
Precommit CI looks like it perhaps has some relevant failures that need to be addressed.
| libcxx/include/typeinfo | ||
|---|---|---|
| 110–111 | ||
| 342 | ||
| libcxx/test/std/language.support/support.rtti/type.info/type_info.equal.pass.cpp | ||
| 11 | I'd also like a test for typeid of a polymorphic type since that's special. e.g., #include <typeinfo>
struct Base {
virtual void func() {}
};
struct Derived : Base {
virtual void func() {}
};
static constexpr Derived D;
constexpr const Base &get_it() {
return D;
}
static_assert(typeid(get_it()) == typeid(Derived));(or something similar) | |
It looks like this is the VCRuntime used by the bots being too old and not supporting constexpr. I think the right thing to do is to UNSUPPORT the test in that case.
@goncharov @mstorsjo Would it be possible to update the VCRuntime on the Windows hosts?
| libcxx/include/typeinfo | ||
|---|---|---|
| 112 | This has to work in pre-C++20 too. | |
| 113 | Since we know what the type is and we control it, we know it doesn't overload operator& so I'd be happy to avoid a dependency on std::addressof. | |
| libcxx/test/std/language.support/support.rtti/type.info/type_info.equal.pass.cpp | ||
| 11 | Addressed both. Aaron, it'd be great if you could have a look to make sure the test I added addresses what you wanted -- I think it does. | |
LGTM assuming assert in the tests is not a problem for some (release) test configurations.
Sorry, I never published my reply to the reviewer's comments.
| libcxx/test/std/language.support/support.rtti/type.info/type_info.equal.pass.cpp | ||
|---|---|---|
| 32–35 | Ahhh, copy-paste. Thanks! | |
| 42 | We don't run the test suite with NDEBUG unless I missed something. TBH this is how all libc++ tests have always been written since its inception, but I do agree this is kinda weird. | |
I checked on a test setup, and the test was failing with VS 2022 17.1, but started after updating it to VS 2022 17.4. And as it's about constexpr, I guess it shouldn't have any runtime aspect (it should run fine even with an older version of the vcruntime DLLs).
I guess it should be possible to update the CI images. As building the docker images usually is a bit messy (it often ends up requiring fixing some dependency which no longer installs cleanly) I guess @goncharov doesn't want to do it all the time - but we should probably do it once Clang 16.0 is released stable so we can include that. I didn't prod him for updating them with Clang 15.0 (so I would believe that they're still running 14.0) since I didn't have any concrete detail that would benefit from that upgrade at the point.
good idea, I am in the process of updating windows images anyway. Hopefully will update within a week. Sorry, I am out of context of this diff, how can I check that new setup passes the test?
Oh, ok.
FWIW, if you do that right now, you'll end up with Clang 15.0 - while 16.0 is to be released within a month. But on the other hand, there shouldn't be much difference (other than that if we update to 15 right now, we'll probably need to update to 17.0 in the fall - while if we update to 16 when that's released, we can in the best cases leave it untouched until next year).
I.e., if you're not in a hurry and want to postpone it for a couple more weeks, that could save yourself some extra work later in the year, but if you have a reason for doing it now anyway, that's totally fine too.
Sorry, I am out of context of this diff, how can I check that new setup passes the test?
IIRC the existing dockerfile tried to install explicitly VS 2019 (whatever is the latest version of that), but we'd need to switch to VS 2022 (which is an entirely new major version). As for inspecting whether the test will succeed or not, you'd have to run the build, remove the UNSUPPORTED: ... line from the test and see how it behaves - that's probably a fair bit of manual poking.
If you can inspect the built container image, you can have a look at C:\BuildTools\VC\Tools\MSVC\<version>\include\vcruntime_typeinfo.h. For current versions, it has something like this:
class type_info
{
public:
[...]
bool operator==(const type_info& _Other) const noexcept
{
return __std_type_info_compare(&_Data, &_Other._Data) == 0;
}While the newer version has got this:
class type_info
{
public:
[...]
_NODISCARD
#if _HAS_CXX23
constexpr
#endif // _HAS_CXX23
bool operator==(const type_info& _Other) const noexcept
{
#if _HAS_CXX23
if (__builtin_is_constant_evaluated())
{
return &_Data == &_Other._Data;
}
#endif // _HAS_CXX23
return __std_type_info_compare(&_Data, &_Other._Data) == 0;
}Or if you can grab the version number of the MSVC installation (the <version> bit in the path above) - from some samples I had lying around, 14.32.31326 is too old while 14.33.31629 seems new enough.