This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use __unwrap_iter_impl for both unwrapping and rewrapping
ClosedPublic

Authored by philnik on Jul 2 2022, 6:42 AM.

Diff Detail

Event Timeline

philnik created this revision.Jul 2 2022, 6:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2022, 6:42 AM
philnik requested review of this revision.Jul 2 2022, 6:42 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 441884.Jul 2 2022, 7:15 AM
  • Try to fix CI
philnik updated this revision to Diff 441934.Jul 3 2022, 3:47 AM
  • Use a tag type for move-only iterators
philnik updated this revision to Diff 441935.Jul 3 2022, 3:51 AM
  • Fix CI
philnik updated this revision to Diff 442141.Jul 4 2022, 1:34 PM
  • Try to fix CI
philnik updated this revision to Diff 442152.Jul 4 2022, 4:19 PM
  • Try to fix CI
philnik updated this revision to Diff 442239.Jul 5 2022, 2:37 AM
  • Next try
philnik updated this revision to Diff 442316.Jul 5 2022, 8:27 AM
  • Next try
Mordante added inline comments.Jul 5 2022, 9:02 AM
libcxx/include/__algorithm/move.h
92 ↗(On Diff #442316)

Please don't use auto, it doesn't help to make the code easier to understand.

libcxx/include/__algorithm/move_backward.h
15 ↗(On Diff #442316)

Why do I need pair.h here. Isn't this exported by unwrap_iter?

libcxx/include/__filesystem/u8path.h
97 ↗(On Diff #442316)

As mentioned in D129040 I really like to avoid using pair when it makes the code unclear. Here it's quite hard to understand what __unwrap_iter(...).first is. Having a good name instead of first will help a lot.

ldionne requested changes to this revision.Jul 7 2022, 9:03 AM

I understand the problem you're solving with move-only iterators and I do think it is worth solving. I'm just not sure how to do it best -- there's something I dislike about the approach taken here, IMO it creates a lot of complexity. I'll take another look and let you know if I can think of another approach.

libcxx/include/__algorithm/copy.h
60

Can you please explain the context around this patch in the commit message? Like you just did verbally during live review.

libcxx/include/__algorithm/unwrap_iter.h
23–24

Can you please update this documentation?

34–35

I like that you are grouping rewrap and unwrap into the same struct. However, we should (not necessarily in this patch) change the name from __unwrap_iter_impl to something else that is more accurate, such as __wrapped_iterator_traits (strawman proposal, this isn't perfect either).

Can you please add a TODO?

This revision now requires changes to proceed.Jul 7 2022, 9:03 AM
philnik retitled this revision from [libc++] Use __unwrap_iter_impl for both unwrapping and rewrapping and allow passing move-only iterators in __unwrap_iter to [libc++] Use __unwrap_iter_impl for both unwrapping and rewrapping.Jul 11 2022, 3:50 PM
philnik updated this revision to Diff 443780.Jul 11 2022, 3:51 PM
philnik marked 2 inline comments as done.
  • Rebased
  • Don't try to allow move-only iterators

I've thought about this some more and came to the conclusion that we shouldn't try to make __rewrap_iter move-only-iterator-friendly. The only places we use that is in copy, copy_backward, move and move_backward. I think the best way to handle this is to forward all these algorithms to copy and unwrap and rewrap everything there. This means that we can unwrap iterators better in all these algorithms and we only have to have the complicated code in one place. The way to forward them all would be to use reverse_iterator for the _backward algorithms and move_iterator for the move algorihtms. The correct way to unwrap to a memcpy would then be to check the return type of *iter and based on that check whether is_trivially_copy_constructible or is_trivially_move_constructible is true. (Forwarding to memcpy with a move_iterator passed to copy might actually be a bug currently)

philnik updated this revision to Diff 443923.Jul 12 2022, 5:29 AM
  • Try to fix CI
philnik updated this revision to Diff 443933.Jul 12 2022, 6:29 AM
  • Next Try
ldionne accepted this revision.Jul 14 2022, 9:08 AM

Forwarding to memcpy with a move_iterator passed to copy might actually be a bug currently

Oof. This is pretty serious, can we please try to address this in priority for LLVM 15? By the way, that's an excellent catch.

libcxx/include/__algorithm/unwrap_iter.h
41

Based on https://eel.is/c++draft/iterator.concept.contiguous and https://eel.is/c++draft/alg.copy#3, it's actually not clear to me that we're allowed to do this, since we're bypassing any potential side effects in the contiguous iterator. I understand that the intent is for implementations to use optimizations on contiguous iterators, however it's not what the spec says, depending on how you read it.

Can you please start a discussion on the LWG reflector asking what the intent is and CC me?

Since this is pre-existing in libc++, I won't block this review on that, but I want to get clarity. If other implementers have consensus on "no we shouldn't be doing this", then we really shouldn't, cause it could be extremely surprising for users. If that's the case, then we should only special-case unwrap_iter for our own libc++ iterators. I hope that's not the case, cause these optimizations are sweet.

libcxx/utils/libcxx/test/params.py
41–42 ↗(On Diff #443933)

Can you please file a GCC bug with a reproducer? Otherwise we'll be stuck with this forever.

This revision is now accepted and ready to land.Jul 14 2022, 9:08 AM