[libcxx-commits] [PATCH] D129040: [libc++] Fix unwrapping ranges with different iterators and sentinels

[Hui via Phabricator via libcxx-commits]

huixie90 accepted this revision.
huixie90 added a comment.

LGTM.

Regarding testing, I am wondering if the tests cover those cases:
iterators that model c++20's random_access_range but only model c++17's InputIterator (this is very common though)
iterators that model c++17's RandomAccessIterator but only model C++20's input_or_output_iterator (or input_iterator)  (this is not very common and I guess we need to define member typedef iterator_category and iterator_concept to make one)

Since the tests in D128611 <https://reviews.llvm.org/D128611> and D128983 <https://reviews.llvm.org/D128983> depend on this fix and currently commented with TODO. Depending the order of which patch land first, we need to update those tests



================
Comment at: libcxx/include/__algorithm/unwrap_range.h:39
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __unwrap(_Iter __first, _Sent __last) {
+    return pair{pair{std::move(__first), nullptr}, std::move(__last)};
+  }
----------------
philnik wrote:
> huixie90 wrote:
> > I'd be really cautious about `nullptr`, which is comparable with pointers. if any of the logic goes wrong, it could result in a runtime UB. Is it possible to avoid `nullptr`?
> How exactly would anything go wrong at runtime? This results in `pair<pair<_Iter, nullptr_t>, _Sent>`. The `nullptr_t` should only ever be passed back to `__rewrap_iter`. If that weren't the case there would be type mismatches all over the place. (i.e. `__rewrap_iter` would just return the unwrapped pointer) But anyways, just to make it //really// hard to break I'll change it to a tag type, since there isn't any downside to do so.
yeah I agree changing it to a tag type is better, at least more obvious to the readers. It is unclear to me what `__rewrap_iter` would do for `nullptr`. Since it is a pair, I would  guess it might compare the first with second (since pointers are comparable with `nullptr_t`). With the tag type it is obvious that it is not going to be compared with first


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129040