This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Correct SFINAE version of is_convertible to match spec and avoid triggering unnecessary compiler diagnostics
Needs RevisionPublic

Authored by ajwong on Jun 28 2014, 2:24 AM.

Details

Reviewers
mclow.lists
Summary

The SFINAE forumulation used to test convertibility did not
correctly hide the conversion failure in ane expression that would
avoid a compiler diagnostic. Switched the idiom to add one more
layer of indirection and use enable_if which fixes the problem.

Also there were a few errors with const-volatile matching causing the
partial specializaitons that handled N2255 to not trigger correctly
for array types.

The following tests now compile in gcc-4.7.

futures.async/async.pass.cpp
map.cons/move_alloc.pass.cpp
map.cons/move_assign.pass.cpp
map.modifiers/insert_iter_rv.pass.cpp
map.modifiers/insert_rv.pass.cpp
meta.rel/is_convertible.pass.cpp
multimap.cons/move_alloc.pass.cpp
multimap.cons/move_assign.pass.cpp
multimap.modifiers/insert_iter_rv.pass.cpp
multimap.modifiers/insert_rv.pass.cpp
pairs.pair/rv_pair_U_V.pass.cpp
pairs.spec/make_pair.pass.cpp
thread.once.callonce/call_once.pass.cpp
unique.ptr.runtime.ctor/default02.pass.cpp
unique.ptr.runtime.ctor/pointer02.pass.cpp
unique.ptr.single.ctor/default02.pass.cpp
unique.ptr.single.ctor/pointer02.pass.cpp
unorder.map.modifiers/emplace_hint.pass.cpp
unorder.map.modifiers/emplace.pass.cpp
unorder.map.modifiers/insert_hint_rvalue.pass.cpp
unorder.map.modifiers/insert_rvalue.pass.cpp
unord.multimap.modifiers/emplace_hint.pass.cpp
unord.multimap.modifiers/emplace.pass.cpp
unord.multimap.modifiers/insert_hint_rvalue.pass.cpp
unord.multimap.modifiers/insert_rvalue.pass.cpp

Diff Detail

Event Timeline

ajwong updated this revision to Diff 10962.Jun 28 2014, 2:24 AM
ajwong retitled this revision from to [libcxx] Correct SFINAE version of is_convertible to match spec and avoid triggering unnecessary compiler diagnostics.
ajwong updated this object.
ajwong edited the test plan for this revision. (Show Details)
ajwong added a reviewer: mclow.lists.
ajwong set the repository for this revision to rL LLVM.
ajwong added a project: deleted.
ajwong added a subscriber: Unknown Object (MLST).
K-ballo added inline comments.
include/type_traits
840

Couldn't __create be replaced by declval, defined earlier in this file?

842

Shouldn't this be named __helper instead?

845

If I understand this correctly, it requires generalized SFINAE to work. Is that the intention?

898

Using add_const/volatile/cv traits would probably be clearer here. Would that have any unwanted consequences?

ajwong updated this revision to Diff 12119.Aug 1 2014, 2:29 PM

Addressed comments. PTAL

gentle ping...

include/type_traits
840

Yes...I wasn't certain why this had __source() previously.

[meta.rel]p4 calls the function create(), but you're right...it looks the same. Changed and added a comment.

842

done.

845

I'm not sure I understand what generalized SFINAE is. I'm testing mainly by compiler.

The previous code just didn't work.

danalbert added a subscriber: danalbert.

Eric speaks template hell. Maybe he can help :)

EricWF edited edge metadata.Aug 20 2014, 8:05 PM

On my TODO list for tonight.

The actual change looks great, and I think its almost ready to land. I would like to see more complete test coverage in meta.rel/is_convertible.pass.cpp . I understand its demonstrated by the other tests passing but I would like a localized test case for the changes.

include/type_traits
834–844

Can you add a test to meta.rel/is_convertible.pass.cpp to test this change?

887–894

I can get meta.rel/is_convertible.pass.cpp passing w/ GCC-TOT if I only apply this part of the change.

EricWF requested changes to this revision.Sep 1 2014, 11:32 PM
EricWF edited edge metadata.

Please implemented the changes requested above.

This revision now requires changes to proceed.Sep 1 2014, 11:32 PM
EricWF resigned from this revision.May 29 2015, 4:05 PM
EricWF removed a reviewer: EricWF.

This review has been inactive for a long time and this bug has already been fixed