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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:48
+    auto __len2    = __last2 - __first2;
+    auto __min_len = __len1 < __len2 ? __len1 : __len2;
+    for (decltype(__min_len) __i = 0; __i < __min_len;) {
----------------
philnik wrote:
> Mordante wrote:
> > Why not using `std::min` here? I assume it is because the types of `__len1` and `__len2` can differ.
> > 
> > I'm not happy with all the `auto`s in this code; it's hard to understand what the intended types are and whether these are the correct intended types. Please remove the `auto`s so it's clear what the types are.
> > 
> > I really would like something along the lines of
> > ```
> > __iter_diff_t<_InputIterator1> __len1 = __last1 - __first1;
> > __iter_diff_t<_InputIterator2> __len2 = __last2 - __first2;
> > auto __min = std::min<common_type_t<decltype(__len1), decltype(__len2)>(__len1, __len2));
> > ```
> > This is more verbose, but at least I can understand how the types are defined and whether this is as intended.
> > 
> I very much disagree with you here. IMO the `__iter_diff_t<_InputIterator1>` and so on just make the code more verbose and give the option to get it wrong. `__last1 - __first1` will always return a difference type. That's not exactly surprising. Also, you want to use `decltype()` for the min for some reason. I guess because otherwise it's //really// long. How does that help with understanding the code?
It helps since the rules of the type of the conditional expression are not simple. I had to verify with the standard it does the right thing. So instead of spending a few seconds to validate these 3 lines I had to spend several minutes.

Code should be optimized for understanding by humans, `auto` quite often saves the writer from typing a few characters. (The compiler doesn't care either way it does its thing.)

The verbose code helps to communicate what the author of the code intended to happen. Relaying on some (not always well understood) language rules means it's less clear for the reader to understand what the writer intended. Both may have a different understanding of these rules.

I also considered to use 3 types
```
using _Len1 = __iter_diff_t<_InputIterator1>;
using _Len2 = __iter_diff_t<_InputIterator2>;
using _Min = common_type_t<_Len1, _Len2>;

_Len1 __len1 = __last1 - __first1;
_Len2 __len2 = __last2 - __first2;

_Min __min = std::min<_Min>(__len1, __len2);
```

I don't mind that solution either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395