[libcxx-commits] [PATCH] D99141: [libcxx] adds `std::incrementable_traits` to <iterator>
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Mar 24 10:40:22 PDT 2021
Mordante added a comment.
Can you also make sure the patch passes CI?
================
Comment at: libcxx/include/iterator:16
namespace std
{
----------------
Can you add the required includes?
http://eel.is/c++draft/iterator.synopsis
```
#include <compare> // see [compare.syn]
#include <concepts> // see [concepts.syn]
```
================
Comment at: libcxx/include/iterator:427
#include <version>
+#include <concepts>
----------------
Maybe also add compare while you're at it.
================
Comment at: libcxx/include/iterator:457
+ requires(const _Tp& __x, const _Tp& __y) {
+ { __x - __y } -> integral;
+ };
----------------
cjdb wrote:
> Quuxplusone wrote:
> > This is the //only// reason `<iterator>` needs to include `<concepts>` in C++20 mode, right?
> > @ldionne, how about we just use `is_integral_v` here instead?
> > ```
> > template<class _Tp>
> > concept __has_integral_minus =
> > !__has_difference_type_member<_Tp> &&
> > requires(const _Tp& __x, const _Tp& __y) {
> > requires is_integral_v<decltype( __x - __y )>;
> > };
> > ```
> >
> > (And/or, wait for `<concepts>` to stop depending on `<iterator>`; D99041, D99124.)
> >
> > I note that the name `__has_integral_minus` is slightly misleading; it currently means `__has_integral_minus_without_difference_type`. The paper standard doesn't factor out these concepts, so we have no exposition-only names to imitate here. Maybe `__incrementable_traits_use_difference_type<_Tp>` and `__incrementable_traits_use_minus<_Tp>` would be better? But I don't really object to the current name even if I //do// find it "slightly misleading."
> >
> > This is the only reason <iterator> needs to include <concepts> in C++20 mode, right?
>
> For this patch? Yes.
>
> It's a part of the [[ http://wg21.link/iterator.synopsis | synopsis ]], so in general, no.
>
> > (And/or, wait for <concepts> to stop depending on <iterator>; D99041, D99124.)
>
> That was my intention, hence "Depends on D99041".
>
> > The paper standard doesn't factor out these concepts, so we have no exposition-only names to imitate here. Maybe `__incrementable_traits_use_difference_type<_Tp>` and `__incrementable_traits_use_minus<_Tp>` would be better?
>
> Right. I think I got the idea of naming these from [[ http://wg21.link/readable.traits | readable.traits ]], so `__has_member_difference_type` (sic) is //probably// best for consistency. As for `__has_integral_minus`, I'm ambivalent, so long as it's consistent.
The synopsis requires concepts http://eel.is/c++draft/iterator.synopsis
================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
http://eel.is/c++draft/incrementable.traits#3 `Users may specialize incrementable_traits on program-defined types.`
can you add a test for this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99141/new/
https://reviews.llvm.org/D99141
More information about the libcxx-commits
mailing list