This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement the resolution of LWG3506 in all language modes.
ClosedPublic

Authored by Quuxplusone on Jul 26 2021, 1:58 PM.

Details

Summary
LWG3506 "Missing allocator-extended constructors for priority_queue"
makes the following changes:
- New allocator-extended constructors for priority_queue.
- New deduction guides targeting those constructors.

Also, do a drive-by fix in the allocator-extended move constructor:
there's no need to do a `make_heap` after moving from `__q.c` into
our own `c`, because that container was already heapified when it
was part of `__q`. [priqueue.cons.alloc] actually specifies the
behavior and does *not* mention calling `make_heap`. I think this
was just a copy-paste thinko. It dates back to the initial import
of libc++.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jul 26 2021, 1:58 PM
Quuxplusone created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 26 2021, 1:59 PM
ldionne requested changes to this revision.Jul 26 2021, 2:10 PM
ldionne added inline comments.
libcxx/include/queue
175–176

Maybe we can get rid of the typename here?

537

I'm not sure I understand why we're constraining those constructors with is_cpp17_input_iterator?

I understand it's just generally a bad idea to have unconstrained template parameters, but that seems to be what the spec has?

554

Doesn't this work even in C++03 mode with the rvalue references extension?

libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons.alloc/ctor_iter_iter_comp_cont_alloc.pass.cpp
22

Why are you creating this derived type?

libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/deduct.pass.cpp
20

This would need to be updated.

This revision now requires changes to proceed.Jul 26 2021, 2:10 PM
ldionne added inline comments.Jul 26 2021, 2:12 PM
libcxx/include/queue
537

Never mind this, I just saw your other patch.

Quuxplusone marked 2 inline comments as done.

Update tests: my sneaky inheritance of constructors didn't work in C++03 mode, so revert to something much closer to the existing approach in other priority_queue tests.
"13.0" is now "14.0".

Quuxplusone marked 2 inline comments as done.Jul 28 2021, 11:14 AM
Quuxplusone added inline comments.
libcxx/include/queue
554

Yes, but I haven't been reckless enough yet to break our historical tradition that guards all actual container move-constructors and move-assignments under _LIBCPP_CXX03_LANG. I think we should break those all at once.

Note particularly the priority_queue(const Comp&, Container&&, const Alloc&) on line 528, which is already guarded like this.

libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons.alloc/ctor_iter_iter_comp_cont_alloc.pass.cpp
22

I'd been doing it in order to get at q.c.get_allocator() on line 35.
However, it turns out that using priority_queue::priority_queue; is a C++11ism, unsupported in C++03 mode, so I'll either have to do the more verbose inheritance from e.g. libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons.alloc/ctor_alloc.pass.cpp or else think of a different way to get at the allocator.

ldionne added inline comments.Jul 28 2021, 12:34 PM
libcxx/include/queue
554

Oh, thanks, I had not realized there was such a tradition.

Quuxplusone marked an inline comment as done.

Fix shadowing warning in new tests by s/comp/compare/g.

ldionne accepted this revision.Jul 28 2021, 1:31 PM

LGTM, but can you please add SFINAE tests for the constructors that you added? I'd like to catch in case we had forgotten to constrain them properly (right now I don't think we would).

This revision is now accepted and ready to land.Jul 28 2021, 1:31 PM
This revision was landed with ongoing or failed builds.Jul 28 2021, 6:16 PM
This revision was automatically updated to reflect the committed changes.