[libcxx-commits] [PATCH] D129039: [libc++] Use __unwrap_iter_impl for both unwrapping and rewrapping

Thu Jul 14 09:08:46 PDT 2022

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

> 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.

================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:41

+// It's a contiguous iterator, so we can use a raw pointer instead
 template <class _Iter>
----------------
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.

================
Comment at: libcxx/utils/libcxx/test/params.py:41-42

+  # -Wstringop-overread seems to be a bit buggy currently
+  '-Wno-stringop-overread',
+
----------------
Can you please file a GCC bug with a reproducer? Otherwise we'll be stuck with this forever.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129039/new/

https://reviews.llvm.org/D129039