Page MenuHomePhabricator

[libcxx] SFINAE-friendly common_type
ClosedPublic

Authored by K-ballo on Jan 14 2015, 5:47 AM.

Details

Summary

Implement a SFINAE-friendly common_type as per LWG2408 for the variadic case, and fixes the no-variadic case to work with void as per LWG1255.

Diff Detail

Event Timeline

K-ballo updated this revision to Diff 18146.Jan 14 2015, 5:47 AM
K-ballo retitled this revision from to [libcxx] SFINAE-friendly common_type.
K-ballo updated this object.
K-ballo edited the test plan for this revision. (Show Details)
K-ballo added reviewers: mclow.lists, EricWF.
K-ballo added a subscriber: Unknown Object (MLST).
K-ballo added inline comments.Jan 14 2015, 5:50 AM
include/type_traits
216

This patch seems to be the first to make use of __void_t. I would rather have this named __void instead as it is not an alias, would that name potentially clash?

1489

Here __common_types is used merely to carry a pack of types, very much like the existing __tuple_types. Perhaps a generic one could be introduced to replace both uses.

K-ballo updated this revision to Diff 18173.Jan 14 2015, 11:59 AM

Updated test to work under C++03 mode

EricWF edited edge metadata.Feb 10 2015, 6:25 PM

LGTM as far as LWG2408. I'll look at LWG1255 tomorrow.

include/type_traits
1487
1496

We can skip an instantiation here and go straight to __common_type2 right?

1501–1502

Isn't there a core language defect about the following specializations being ambiguous? Something along the lines of this?

template <class ...> struct Foo;

template <class T> 
struct Foo<T> {}; // Specialization 1

template <class T, class ... Args>
struct Foo<T, Args...> {}; // Specialization 2

using MyFoo = Foo<int>; // The defect is that both 1 and 2 match.

As far as I know it is accepted by all compilers but I just wanted to bring this up.

K-ballo added inline comments.Feb 11 2015, 6:34 AM
include/type_traits
1487

That's right, the current wording with only 3 bullets regresses key functionality when it comes to user-defined specializations of common_type. The 4 bullet version was my suggestion to get the original behavior back. This is now being tracked instead by LWG2465, so we could wait for it to settle to move forward with this patch. Otherwise, I'd rather not have a sfinae-friendly result_of if that means regressing functionality.

1496

No, doing so would mean skipping over user-defined specializations of common_type

1501–1502

I'm not sure, will investigate.

Just so I'm sure I understand: This implementation applies decay<T> for ever pair of types in common_type<T...> even though N2408 says decay is only applied to the final type?

include/type_traits
1478

I think we need to qualify the declval<Tp> calls so they don't use ADL.

1501–1502

I think I found the bug that tracks this: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1432 . Could we work around it for now?

Even though this in no-where close to perfect (because we don't know how to specify common_type) I think this is a strict improvement to what we have now for 3 reasons:

  1. It makes common_type SFINAE friendly.
  2. It properly supports common_type<void, void>
  3. It properly supports user defined specializations (unlike N2408).

I'm can't decide on whether we should keep applying decay to the result of every ternary expression or adopt the new behaviour of only decaying the final result. However since this patch maintains the existing behaviour I think we should apply it until the committee comes to a consensus about what to do.

@K-ballo: Are you still willing to work on this?

include/type_traits
1453

Is it possible to make the non-variadic version SFINAE friendly as well? Or does it require expression SFINAE (which ilslikely not available)?

Just so I'm sure I understand: This implementation applies decay<T> for ever pair of types in common_type<T...> even though N2408 says decay is only applied to the final type?

That sounds correct. In other words, it applies as much decay and in the same places as C++11 common_type used to do.

@K-ballo: Are you still willing to work on this?

I am willing but time is lacking, and I'm already on my way to Lenexa. If you know how to workaround CWG1432 and want to move this patch forward quickly then don't let me hold it.

include/type_traits
1453

The implementation used in the variadic version does indeed require expression SFINAE.

1478

Good catch.

1501–1502

I'm not sure I understand how to workaround this issue. Would that just take a third non-variadic argument, say <_Tp, _Up, _Vp, Wp...>?

I am willing but time is lacking, and I'm already on my way to Lenexa. If you know how to workaround CWG1432 and want to move this patch forward quickly then don't let me hold it.

Right! I forgot that the US is quite a trek for you. I'll see you in Lenexa. I'm not worried about moving this quickly I just feel bad for leaving this for so long. Sorry : (

include/type_traits
1501–1502

Yeah I think that is how you work around it. However I just noticed that our current common_type has this problem and it hasn't broken yet.

EricWF accepted this revision.Sep 7 2015, 4:48 PM
EricWF edited edge metadata.

LGTM. I don't know why I've been putting this off for so long.

There is still some contention over weither common_type should apply "decay" to every result or just the last one. This patch applies it to every result as does the current implementation.
For that reason I think this is a strict improvement.

@K-ballo May I commit this with a couple of small changes? I want to fully qualify all of the calls to "declval<Tp>(...)".

This revision is now accepted and ready to land.Sep 7 2015, 4:48 PM

@K-ballo May I commit this with a couple of small changes? I want to fully qualify all of the calls to "declval<Tp>(...)".

Please do. I have submitted bug reports in the past for pesky unqualified calls to declval.

EricWF closed this revision.Sep 7 2015, 5:16 PM

Committed in r246977. Thanks again.