This is an archive of the discontinued LLVM Phabricator instance.

Make common_type's implementation common
ClosedPublic

Authored by EricWF on Mar 21 2019, 5:08 PM.

Details

Summary

Currently the C++03 implementation of common_type has much different behavior than the C++11 one. This causes bugs, including inside <chrono>.

This patch unifies the two implementations as best it can. The more code they share, the less their behavior can diverge.

Diff Detail

Repository
rCXX libc++

Event Timeline

EricWF created this revision.Mar 21 2019, 5:08 PM

Very impressive. Still looking.

include/type_traits
2121

Why this define? It's only used one other place - why not just use _LIBCPP_CXX03_LANG?

EricWF marked 2 inline comments as done.Mar 21 2019, 6:15 PM
EricWF added inline comments.
include/type_traits
2121

Woops, I meant to clean that up.

EricWF updated this revision to Diff 191811.Mar 21 2019, 6:15 PM
EricWF marked an inline comment as done.
mclow.lists added inline comments.Mar 21 2019, 6:24 PM
test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
327

Any reason not to use ASSERT_SAME_TYPE in these tests? I find it much easier to read.

EricWF marked 2 inline comments as done.Mar 21 2019, 6:26 PM
EricWF added inline comments.
test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
327

Not really, I was just going with the existing style of the file. Normally I try to write ASSERT_SAME_TYPE as well.

ldionne added inline comments.Mar 21 2019, 7:35 PM
include/type_traits
2189

If one is already decayed but the other isn't, shouldn't you decay the one that isn't?

EricWF marked 2 inline comments as done.Mar 21 2019, 8:16 PM
EricWF added inline comments.
include/type_traits
2189

No. The standard is very specific about this:

http://eel.is/c++draft/meta.trans.other#3.3

mclow.lists added inline comments.Mar 21 2019, 8:20 PM
test/std/utilities/meta/meta.trans/meta.trans.other/common_type.pass.cpp
327

My policy these days is that any test I touch gets updated to use ASSERT_SAME_TYPE and ASSERT_NOEXCEPT.

ldionne accepted this revision.Mar 22 2019, 12:14 PM

This is good by me (but you should address Marshall's comment about the tests). Cool trick with the macro for variadic templates.

include/type_traits
2189

Ah, I see! To match the wording perfectly you could also do:

conditional<
    !is_same<_Tp, typename decay<_Tp>::type>::value || !is_same<_Up, typename decay<_Up>::type>::value,
    common_type<typename decay<_Tp>::type, typename decay<_Up>::type>,
    __common_type2_imp<_Tp, _Up>
>

I'm not requesting that you do that, though.

This revision is now accepted and ready to land.Mar 22 2019, 12:14 PM
mclow.lists accepted this revision.Mar 25 2019, 10:40 AM

This is good by me (but you should address Marshall's comment about the tests). Cool trick with the macro for variadic templates.

I told Eric (privately) that I was fine with him cleaning up the test in a separate commit.

This revision was automatically updated to reflect the committed changes.