This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix __wrap_iter to be a proper contiguous iterator.
ClosedPublic

Authored by Quuxplusone on Sep 21 2021, 1:51 PM.

Details

Summary

The latest installment in the ever-evolving saga of __cpp17_contiguous_iterator, __wrap_iter, and __unwrap_iter...

Instead of overloading __to_address, let's specialize pointer_traits. Function overloads need to be in scope at the point where they're called, whereas template specializations do not. (User code can provide pointer_traits specializations to be used by already-included library code, so obviously __wrap_iter can do the same.)

pointer_traits<__wrap_iter<It>> cannot provide pointer_to, because you generally cannot create a __wrap_iter without also knowing the identity of the container into which you're trying to create an iterator. I believe this is OK; contiguous iterators are required to provide to_address but *not* necessarily pointer_to.

I'm less sure about whether pointer_traits must provide rebind... but I don't see how it could possibly be implementable, so I think it'll be OK to omit it.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Sep 21 2021, 1:51 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 21 2021, 1:51 PM
jloser added inline comments.Sep 21 2021, 4:36 PM
libcxx/include/__iterator/wrap_iter.h
300

Question: can you explain where the naming comes from/what it stands for? I see precedence for this naming in type_traits, so it's fine as-is given its use below, but just curious.

libcxx/include/__iterator/wrap_iter.h
300

I believe __nat stands for "Not A Type."
The shape of this specialization was all just copied from the pointer_traits<_Tp*> specialization, btw.

jloser accepted this revision.Sep 22 2021, 8:57 AM

Seems reasonable to me. Thanks for digging into this!

ldionne requested changes to this revision.Sep 22 2021, 10:01 AM

This is much nicer, thanks!

Can you please add tests in test/libcxx for pointer_to? It's not clear to me that it works at all.

libcxx/include/__iterator/wrap_iter.h
287

I think we should only specialize for __wrap_iter<_Tp*>, or perhaps for any _It that is a contiguous iterator. Otherwise, we're not guaranteed that it's legal to call to_address(__w.base()) below. WDYT?

307
This revision now requires changes to proceed.Sep 22 2021, 10:01 AM
Quuxplusone edited the summary of this revision. (Show Details)

Remove pointer_traits<__wrap_iter>::rebind and ::pointer_to.

Quuxplusone marked an inline comment as done.Sep 22 2021, 10:44 AM
Quuxplusone added inline comments.
libcxx/include/__iterator/wrap_iter.h
287

We already assume that _It must be a contiguous iterator; see line 283, also line 36. This was the outcome of that whole series of PRs among Zoe and myself that finally sorted out that yes __wrap_iter only ever actually wraps pointers, and it would break horribly if you ever manually tried to wrap anything that wasn't a pointer, so let's just stop pretending that it can.

293–296

@ldionne wrote:

Can you please add tests in test/libcxx for pointer_to? It's not clear to me that it works at all.

You're right, it was totally busted — and rebind was busted worse! :P I'd just copied that code over without looking at it. It's gone now. I think, but am not sure, that this is still conforming. I don't think client code can conformingly rely on being able to look at pointer_traits<vector<int>::iterator>, let alone pointer_traits<vector<int>::iterator>::rebind<const int> or whatever. This pointer_traits is just an implementation detail that gets us the enabled std::to_address that we actually want to be client-visible.

ldionne accepted this revision.Sep 22 2021, 11:18 AM

LGTM except for the weird includes, and also I'd like to have a test that exercises std::to_address for a __wrap_iter. I suspect the #include confusion started as an attempt to add such a test?

I don't need to see this again unless it changes significantly. Thanks a bunch for looking into that!

libcxx/include/__iterator/wrap_iter.h
287

Sorry, I meant to delete my comment before submitting the review. You're right, I remembered that after writing it.

293–296

Yeah.. I don't think that specialization of pointer_traits would be conforming for a user defined type, but we can do it cause we're the implementation.

libcxx/test/std/utilities/memory/pointer.traits/pointer_to.pass.cpp
20–23

Why those includes?

This revision is now accepted and ready to land.Sep 22 2021, 11:18 AM
Quuxplusone marked 3 inline comments as done.Sep 22 2021, 11:21 AM
Quuxplusone added inline comments.
libcxx/test/std/utilities/memory/pointer.traits/pointer_to.pass.cpp
20–23

Oops. I had originally added some test cases here for pointer_traits<std::vector<int>::iterator>::pointer_to etc., before eventually realizing "oh yeah, that'll never work." And then I reverted that part of the file, but forgot to revert the includes.

Quuxplusone marked an inline comment as done.Sep 22 2021, 11:23 AM
Quuxplusone added inline comments.
libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
13–16

@ldionne wrote:

also I'd like to have a test that exercises std::to_address for a __wrap_iter.

That's what this is... and now it passes in debug mode too. :) This suffices, right?

LGTM with include fixes and green CI!

libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
13–16

Oh, sorry, yeah. Went too fast.

Remove superfluous includes. If buildkite likes this, I'll land it.

This revision was landed with ongoing or failed builds.Sep 22 2021, 3:52 PM
This revision was automatically updated to reflect the committed changes.