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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
Comment at: libcxx/include/__algorithm/lexicographical_compare_three_way.h:46
+    }
+    return __len1 <=> __len2;
+  } else {
----------------
mumbleskates wrote:
> philnik wrote:
> > JohelEGP wrote:
> > > huixie90 wrote:
> > > > agree with @philnik that providing benchmark to show the difference.
> > > > 
> > > > question: the standard specifies `return (last1 - first1) <=> (last2 - first2);` but here you are using `size_t` instead of iterator's `difference_type`. so effectively
> > > > `return static_cast<size_t>(last1 - first1) <=> static_cast<size_t>(last2 - first2);`. Does this have an observable difference?
> > > It should if the difference type is an integer-class type.
> > I don't think it has any observable effects, but we shouldn't cast to different types, since that can prohibit some optimizations. 
> i'm not even 100% sure what is allowed to be assumed about `difference_type`; i would definitely avoid using `size_t` here and stick entirely to the iterator's difference type.
Using `difference_type` now, and also only using this optimization if both iterator types have the same, integral `difference_type`


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:110-118
+  // Both iterator types are random access iterators -> triggers a special fast path inside `lexicographical_compare_three_way`
+  test_iterator_types<const int*, const int*>();
+  test_iterator_types<contiguous_iterator<const int*>, cpp20_random_access_iterator<const int*>>();
+  // One of the two iterator types is not a random access iterator -> fallback to slower algorithm
+  test_iterator_types<const int*, forward_iterator<const int*>>();
+  test_iterator_types<forward_iterator<const int*>, const int*>();
+  // Check with various other iterator types
----------------
philnik wrote:
> avogelsgesang wrote:
> > philnik wrote:
> > > Please check the complete cartesian product of `cpp17_input_iterator`, `forward_iterator`, `bidirectional_iterator`, `random_access_iterator`, `contiguous_iterator`, `const int*` and `int*`. Also, `cpp20_random_access_iterator` is just a C++17 input iterator.
> > done, except for `int*`. `int*` does not work, because the currently used `array`s are const.
> > Is it fine not to test `int*`? Which additional test coverage would `int*` give us over `const int*`?
> You can just take the arrays by value instead of const reference. `int*` checks that the algorithms doesn't fail for mutable types. It's not expected that it will happen, but it's also a very common scenario and quite easy to add tests for.
ok, also added tests for `int*`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395