[libcxx-commits] [PATCH] D150493: [libc++] Add functions that ensure a given iterator meets the syntactic requirements

Tue May 16 11:59:08 PDT 2023

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.

================
Comment at: libcxx/include/__iterator/check_iterators.h:51
+template <class _Iterator>
+_LIBCPP_CONSTEXPR void __check_iterator_requirements() {
+  static_assert(__has_dereference_operator<_Iterator&>::value, "Iterator has to be dereferenceable");
----------------
ldionne wrote:
> I am a bit uneasy about adding yet another way to check for iterator-ness, since we already have `__is_cpp17_forward_iterator` and the `forward_iterator` concept in C++20. Let's discuss this some more later when we have more time.
Ok, suggestion per our discussion:

1. Refactor `__is_cpp17_bidirectional_iterator` & friends to properly convey that they are just checking the iterator category and they're not acting like a proper concept.
2. Introduce `__cpp17_bidirectional_iterator` as a `inline constexpr bool` that check these concepts (suggestion: move those to `libcxx/include/__iterator/concepts.h` or similar)
3. Then do `static_assert(__cpp17_bidirectional_iterator<_It>)` in the PSTL algorithms

Does that sound like a plan? I think what I mostly disliked from this as-is is that we were introducing what looked like yet another way to tell whether a type satisfied an iterator concept for C++17, but after (1) this shouldn't be an issue anymore.

We could make (2) be a `concept` instead of `constexpr bool` in C++20 so that we get nicer diagnostics for users on C++20 when the static assert fails. I'd also be OK with implementing (2) as concepts directly and enabling those QOI checks only in C++20 mode. After all, those are concept checks and concept checks shouldn't really be expected before concepts exist.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150493