[libc++] std::prev(it) should not compile for a non-bidi iterator

[ldionne]

std::prev(it, n) could conceivably be well-formed for a non-bidirectional it if n is negative, however std::prev(it) is always UB, and we can easily diagnose it at compile-time. Right now std::prev(non_bidi) is a no-op, which is absolutely vexing.

https://godbolt.org/z/zaG3jTEqP

Potential patch:

diff --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h
index e950d8dc4147..98883188312a 100644
--- a/libcxx/include/__iterator/prev.h
+++ b/libcxx/include/__iterator/prev.h
@@ -26,7 +26,7 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _InputIter, __enable_if_t<__has_input_iterator_category<_InputIter>::value, int> = 0>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter
-prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n = 1) {
+prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n) {
   // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation.
   // Note that this check duplicates the similar check in `std::advance`.
   _LIBCPP_ASSERT_PEDANTIC(__n <= 0 || __has_bidirectional_iterator_category<_InputIter>::value,
@@ -35,6 +35,12 @@ prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n =
   return __x;
 }
 
+template <class _BidirectionalIterator,
+          __enable_if_t<__has_bidirectional_iterator_category<_BidirectionalIterator>::value, int> = 0>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _BidirectionalIterator prev(_BidirectionalIterator __it) {
+  return std::prev(std::move(__it), 1);
+}
+
 #if _LIBCPP_STD_VER >= 20
 
 // [range.iter.op.prev]

[philnik777]

I think it would be better to static_assert here, since this isn't a constraint.

[frederick-vs-ja]

There's LWG3197 for this, which is still open now.

[ldionne]

I think it would be better to static_assert here, since this isn't a constraint.

Per the LWG issue mentioned by @frederick-vs-ja , it seems like it's not clear to everyone whether there is a constraint or not.

IMO this should definitely be a constraint, prev implies going backwards, and I would argue that 99% of the usage of std::prev with a non-bidirectional iterator is probably unintended.

[ldionne]

CC @jwakely perhaps we want to bring back that LWG issue to the table?

[jwakely]

It's been on the table for five years 😄 apart from the note I added a few months ago nobody had made a case for why we should prefer any of the options.

[ldionne]

I would argue for option (A) or option (C) in LWG3197, otherwise the result is that calling std::prev(non_bidi) has undefined behavior. Since it's so easy to catch and I think we can agree that most users don't intend to call std::prev to go forward, we're likely going to catch unintended misuse without preventing any significant intended use.

(That's me trying to make a case BTW 🙂)