[libcxx-commits] [PATCH] D131395: [libc++] Implement `lexicographical_compare_three_way`

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added a comment.

I think I addressed all actionable review comments now.

There is one unresolved, controversial comments remaining, though: `auto` vs explicit types:

- @mordante prefers explicit types <https://reviews.llvm.org/D131395#anchor-inline-1273865>
- @philnik prefers auto <https://reviews.llvm.org/D131395#anchor-inline-1273889>
- @mumbleskates first preferred auto <https://reviews.llvm.org/D131395#anchor-inline-1272748> but now seems to prefer explicit types <https://reviews.llvm.org/D131395#anchor-inline-1273952>

Given that there are good arguments in both directions and I don't see one side as objectively better, I would propose to merge the commit as currently in review, in the interest of making progress.
Can you live with that, @mumbleskates, @philnik, @mordante?



================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:44-45
+                  "A non-integral difference_type is undefined behavior");
+    static_assert(is_integral_v<typename iterator_traits<_InputIterator2>::difference_type>,
+                  "A non-integral difference_type is undefined behavior");
+    auto __len1    = __last1 - __first1;
----------------
avogelsgesang wrote:
> avogelsgesang wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > At Discord we discovered concepts give nicer error messages. Can you test whether that's true here too, by using `integral` instead of `is_integral_v`?
> > > > 
> > > > Since you use two asserts, lets give additional information; alternatively merge them in one assert.
> > > Instead of having a lot of `static_assert`s here, maybe we want to add the exposition only concepts for `Cpp17Iterator`s from the standard somewhere and `static_assert` that here?
> > > Can you test whether that's true here too, by using integral instead of is_integral_v?
> > 
> > https://godbolt.org/z/rKdPq9joo
> > 
> > Given that we implemented the `integral ` as
> > ```
> > concept integral = is_integral_v<_Tp>;
> > ```
> > 
> > using a concept here doesn't improve the error message. It only adds a more complicated backtrace to the error message which does not really provide much value.
> > [...] maybe we want to add the exposition only concepts for Cpp17Iterators from the standard somewhere and static_assert that here?
> 
> You mean
> 
> ```
> static_assert(__cpp17_iterator<_InputIterator1>, "calling lexicographical_compare_three_way with a non-standard-compliant iterators is undefined behavior");
> static_assert(__cpp17_iterator<_InputIterator2>, "calling lexicographical_compare_three_way with a non-standard-compliant iterators is undefined behavior");
> ```
> 
> ?
To me, the goal of the

```
static_assert(is_integral_v<typename iterator_traits<_InputIterator1>::difference_type>,
              "Using a non-integral difference_type is undefined behavior");
static_assert(is_integral_v<typename iterator_traits<_InputIterator2>::difference_type>,
              "Using a non-integral difference_type is undefined behavior");
```

was to remind the reader: the `difference_type` is an integer, keep that in mind while reading the following code. Replacing this by a `__cpp17_input_iterator` assert would no longer fulfil this purpose


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395