Page MenuHomePhabricator

[libcxx][type_traits] Add C++20 changes to common_type
ClosedPublic

Authored by miscco on Feb 10 2020, 11:18 AM.

Details

Summary

This already implements the expected changes for LWG-3205

Diff Detail

Event Timeline

miscco created this revision.Feb 10 2020, 11:18 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 8 2020, 11:39 AM
EricWF added a comment.Apr 8 2020, 3:11 PM

I assume this is a behavioral change? Why are there no new tests?

Actually I was having trouble comming up with a test case for bullet 4.

miscco updated this revision to Diff 256389.Apr 9 2020, 1:31 PM

Add test for a reference wrapper type

miscco added a comment.Apr 9 2020, 1:32 PM

Thanks to @CaseyCarter for the example

ldionne accepted this revision.May 7 2020, 9:21 AM

LGTM, but why is there no change to the conformance status page?

This revision is now accepted and ready to land.May 7 2020, 9:21 AM

Thanks for the review, could somebody commit it as I do not have the rights

Thanks for the review, could somebody commit it as I do not have the rights

Doing -- but could you please ask for commit rights? This wouldn't prevent you from needing to request reviews, but at least you could commit your patches yourself once you have the LGTM. The process for obtaining commit rights is https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

ldionne requested changes to this revision.May 14 2020, 5:23 AM

Sorry, two more things as I look at the diff again locally.

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
228

There's a weird non-printable character on this line (<U+00AD>). I noticed that by looking at the diff in git. Please watch out for those.

231

Don't we need to test with some cv qualified and ref types too?

This revision now requires changes to proceed.May 14 2020, 5:23 AM
miscco marked 2 inline comments as done.May 14 2020, 12:44 PM
miscco added inline comments.
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
228

Sory, I guess this comes from copying from eel.is I will have a look for those

231

I guess adding cv qualifiers similar to the above bullets should work even for a reference wrapper.

If I understand CREF correctly this should also work std::is_same_v<std::common_type_t<bad_reference_wrapper<double>&, double>, double>

I will come back with some additional tests

miscco updated this revision to Diff 264222.May 15 2020, 6:19 AM

Fix incorrect symbol in copied text and add some more tests

I really "love" arcanist. Gonna check why it is adding unrelated stuff again

miscco marked 2 inline comments as done.May 15 2020, 6:22 AM

@ldionne I added additional tests and removed the spurious character

ldionne accepted this revision.May 15 2020, 6:32 AM
This revision is now accepted and ready to land.May 15 2020, 6:32 AM
This revision was automatically updated to reflect the committed changes.