This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P1328R1 constexpr std::type_info::operator==
ClosedPublic

Authored by ldionne on Feb 6 2023, 5:10 PM.

Diff Detail

Event Timeline

ldionne created this revision.Feb 6 2023, 5:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 5:10 PM
ldionne requested review of this revision.Feb 6 2023, 5:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 5:10 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.

ldionne updated this revision to Diff 495329.Feb 6 2023, 5:13 PM

Update synopsis

ldionne updated this revision to Diff 495364.Feb 6 2023, 8:10 PM

Rebase onto main

Mordante accepted this revision.Feb 7 2023, 12:56 AM
Mordante added a subscriber: Mordante.

LGTM, modulo some nits.

libcxx/include/typeinfo
112

Just curious, but why __libcpp_is_constant_evaluated instead of std::is_constant_evaluated?

113
libcxx/test/std/language.support/support.rtti/type.info/type_info.equal.pass.cpp
12

Can you add a test for noexcept too?

This revision is now accepted and ready to land.Feb 7 2023, 12:56 AM

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
12

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)

ldionne marked 6 inline comments as done.Feb 7 2023, 8:02 AM
ldionne added subscribers: goncharov, mstorsjo.

Precommit CI looks like it perhaps has some relevant failures that need to be addressed.

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
12

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.

ldionne updated this revision to Diff 495548.Feb 7 2023, 8:03 AM
ldionne marked 3 inline comments as done.

Address comments.

aaron.ballman added inline comments.Feb 7 2023, 8:34 AM
libcxx/test/std/language.support/support.rtti/type.info/type_info.equal.pass.cpp
12

I think it does as well

32–35

These are unused in the test.

42

I'm very unfamiliar with libc++ testing patterns, but does something ensure that the assert macro actually expands to test things even in NDEBUG builds?

ldionne updated this revision to Diff 495601.Feb 7 2023, 10:57 AM
ldionne marked 3 inline comments as done.

Fix C++03 and remove dead code.

aaron.ballman accepted this revision.Feb 7 2023, 12:17 PM

LGTM assuming assert in the tests is not a problem for some (release) test configurations.

Mordante accepted this revision.Feb 7 2023, 12:28 PM

Thanks LGTM!

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.

This revision was landed with ongoing or failed builds.Feb 7 2023, 5:04 PM
This revision was automatically updated to reflect the committed changes.

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?

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.

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?

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?

good idea, I am in the process of updating windows images anyway. Hopefully will update within a week.

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.