This is an archive of the discontinued LLVM Phabricator instance.

Implement P0433: deduction guides for <set>
ClosedPublic

Authored by Quuxplusone on Feb 23 2019, 12:37 PM.

Diff Detail

Repository
rCXX libc++

Event Timeline

Oops, fix a cut-and-paste thinko in the test for multiset.

Use ASSERT_SAME_TYPE. Better comments in the .fail.cpp file.

EricWF requested changes to this revision.Mar 4 2019, 6:40 PM
EricWF added a subscriber: EricWF.

Could you please specify what paper you're implementing in the description?

test/std/containers/associative/set/set.cons/deduct.pass.cpp
43

clang-format any new files please.

This revision now requires changes to proceed.Mar 4 2019, 6:40 PM
Quuxplusone retitled this revision from Implement deduction guides for <set> to Implement P0433: deduction guides for <set>.Mar 4 2019, 7:25 PM
Quuxplusone edited the summary of this revision. (Show Details)
Quuxplusone marked an inline comment as done.

clang-format'ed.

Rebased on master.

@EricWF ping?

(Is "CTAD for container types" similar to "polymorphic memory resources" in the bucket of "things libc++ doesn't want to implement for engineering reasons," and if so, could you guys go on record with that? It would be less dysfunctional than the current model.)

Rebased on master.

@EricWF ping?

(Is "CTAD for container types" similar to "polymorphic memory resources" in the bucket of "things libc++ doesn't want to implement for engineering reasons," and if so, could you guys go on record with that? It would be less dysfunctional than the current model.)

No, it is not like that.

I've just been quite busy. Please ping this weekly to help it gain attention. I'll try to take a look at it this week.

A couple of minor nits.
Same comment about __identify as in the other review. You need answer it in one place, only.

test/std/containers/associative/multiset/multiset.cons/deduct.pass.cpp
89

A comment here "braces instead of paren" would be nice.

105

These should all be const

Almost there. A couple nits, a few consts, and landing D58587, and we're done.

include/set
858

Any reason that you're not using __iter_value_type here? (Besides that it is declared in D58587)

1357

same comment re: __iter_value_type

test/std/containers/associative/set/set.cons/deduct.pass.cpp
95

Please change to std::allocator<long> When we tighten up the constraints on set, it won't break this test.

Quuxplusone marked 5 inline comments as done.May 29 2019, 9:24 AM
Quuxplusone added inline comments.
include/set
858

No, I think just that it's declared in D58587. I was thinking of <set> as the first thing to land and then <map> after that, but I see how it might make more sense to land <map> first, with all the helper aliases, and then rebase <set> on top of it.

Quuxplusone marked an inline comment as done.

Review comments. (Add "braces instead of parens" comment; add const to int expected_s[] throughout)

Update the tests to no longer test P1518R0 (although it is still supported by the code); this is the same thing I did in my last edit to D58587.

@mclow.lists ping! I think this one is now ready to land, unless I missed something.

ldionne accepted this revision.Jun 11 2019, 8:31 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 11 2019, 11:20 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 11:20 AM