This is an archive of the discontinued LLVM Phabricator instance.

[C++23] [P1518R2] Better CTAD behavior for containers with allocators
ClosedPublic

Authored by Quuxplusone on Mar 1 2021, 4:05 PM.

Details

Summary

P1518 does the following in C++23 but we'll just do it in C++17 as well:

  • Stop requiring Alloc to be an allocator on some container-adaptor deduction guides
  • Stop deducing from Allocator on some sequence container constructors
  • Stop deducing from Allocator on some other container constructors (libc++ already did this)

The affected constructors are the "allocator-extended" versions of constructors where the non-allocator arguments are already sufficient to deduce the allocator type. For example,

std::pmr::vector<int> v1;
std::vector v2(v1, std::pmr::new_delete_resource());
std::stack s2(v1, std::pmr::new_delete_resource());

P1518R2 (which will be in the next mailing) was adopted, or at least resolved-to-be-adopted, by LWG the other day.

I'm proposing that libc++ implement these CTAD improvements not only in C++2b mode, but also in all modes (i.e. C++17 and C++20). Anyone who doesn't notice won't mind, and anyone who does notice will thank us. Plus, libc++ already implements a little more than half of P1518R2 in all modes. This patch is just getting us all the way there.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Mar 1 2021, 4:05 PM
Quuxplusone created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 1 2021, 4:05 PM

This mostly LGTM but I haven't read it too carefully. Thanks for doing this, Arthur.

libcxx/include/deque
1318

I think all the compilers we that support support templated type aliases. WDYT about making this deque(const deque& __c, const __identity_t<allocator_type>& __a);?

Quuxplusone added inline comments.Mar 2 2021, 6:12 AM
libcxx/include/deque
1318

Indeed, I'm 99.9% sure we can use alias templates, because of _EnableIf.
But right now libc++ has no __identity_t alias. If we were going to add one, I think I'd lobby for it to be like _Ident<T>, by analogy with _EnableIf<B>, just to keep the boilerplate as short as possible. (Although _EnableIf has advantages in minimizing helper template instantiations, whereas this thing would not.)

zoecarver added inline comments.Mar 2 2021, 9:22 AM
libcxx/include/deque
1318

_Ident<T> sounds good to me :)

I know this doesn't have compile-time benefit, but I think it will make the code a bit cleaner. Just a small thing.

Quuxplusone marked an inline comment as done.Mar 3 2021, 8:24 AM
Quuxplusone added inline comments.
libcxx/include/deque
1318

Posted D97862. I went with __identity_t after all, because there are some irreducible uses of __identity and I didn't want to have, like, __identity<T> and _Ident<T> with subtly different meanings. (Even if _EnableIf is precedent! ;))

Please consider upgrading the status page.

zoecarver added inline comments.Mar 3 2021, 2:29 PM
libcxx/include/deque
1318

And uses fewer characters ;)

ldionne accepted this revision.Mar 3 2021, 3:11 PM

I spoke to Arthur about this offline. We usually don't implement stuff before it gets into the Standard, and I'd like to avoid creating a precedent. Waiting for it to get into the standard also means that we won't be caught off-guard if the paper changes at the last minute (which happens), and it will simplify our book-keeping by not having to remember to go back and update the Status list after the paper lands. So instead, Arthur and I agreed that we could pre-review this and when it's ready, he will just sit on it until the paper is actually voted in plenary, and then he can commit it immediately.

We also normally don't implement extensions (e.g. back-porting a paper to an earlier standard), and there are good reasons for that (it leads to tech debt, bugs down the road and we sometimes even break users in subtle ways). However, in this case, I think it's really reasonable to implement in previous standards:

  1. This is basically fixing a defect that we hadn't noticed in previous standards, so it's akin to a LWG issue IMO.
  2. The only way this can hurt someone is if they somehow rely on the use cases enabled by the paper not working in a SFINAE context (or at least I can't think of something else). I have little sympathy for these sorts of breakages - otherwise we'd be prevented from making basically any change.
  3. Fixing the issue in previous standards increases consistency and reduces code complexity (in the test suite too), so we don't lose anything here.

The change itself looks good to me, but we'll have to edit the Status page upon landing it.

This revision is now accepted and ready to land.Mar 3 2021, 3:11 PM
Quuxplusone marked an inline comment as done.

Added a "NFCI" commit in front of my feature work here:

[libc++] Use _EnableIf and __iter_value_type consistently. NFCI.

Specifically, use these metafunctions consistently in areas that are
about to be affected by P1518R2's changes.

Rebase on main. Add tests for LWG3025 and LWG3531. (We don't need any changes for LWG3531 AFAICT. LWG3531 says to do using value_type = __identity_t<pair<...>>, but that doesn't seem to affect anything in the relevant test cases. Being unable to demonstrate the point of that change, I haven't made it, and have emailed LWG for clarification instead.)

Landed the NFCI part; rebased on main. Now this PR shows only the functional change again.

Update Cxx2bStatusPaperStatus.csv, and poke buildkite. The paper has been merged
https://github.com/cplusplus/draft/pull/4672
so if buildkite is still happy with this, imma land it.

Minor nit-pick found while reading e-mail, haven't looked at the rest of the patch.

libcxx/docs/Cxx2bStatusPaperStatus.csv
5

Minor nit-pick the placement seems odd. This should be a new section for the June 2021 accepted papers.
(I'm not sure who adds the papers and issues after a meeting, but it would be nice to have all papers and issues listed.)

libcxx/docs/Cxx2bStatusPaperStatus.csv
5

it would be nice to have all papers and issues listed

It sure would. @ldionne? :)

This should be a new section for the June 2021 accepted papers.

Hm, I had blindly figured they were just listed asciibetically by paper number, but yeah, I see Cxx2aStatusPaperStatus.csv is organized in the way you describe. Will fix.

Not sure what happened last time to make "patch application failed", but let's just try again.

Ah, it was perhaps something to do with the existence of a parent revision (even though the parent revision was already merged and closed out).