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

[Hui via Phabricator via libcxx-commits]

huixie90 accepted this revision as: huixie90.
huixie90 added a comment.

LGTM with nits. I will leave the final approval to others who also commented



================
Comment at: libcxx/benchmarks/lexicographical_compare_three_way.bench.cpp:35
+// `int*` is a random access iterator while `cpp17_input_iterator` is not.
+BENCHMARK_TEMPLATE(BM_lexicographical_compare_three_way, int*)->RangeMultiplier(2)->Range(1, 1 << 20);
+BENCHMARK_TEMPLATE(BM_lexicographical_compare_three_way, cpp17_input_iterator<int*>)
----------------
nit: you can use random_access_iterator<int*> from the test_iterators.h so that both tests are using some iterator wrappers (to be more fair). It also saves you to explain that `int*` is a random access iterator


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way.pass.cpp:121
+  test_given_iterator_types<contiguous_iterator<const int*>, random_access_iterator<const int*>>();
+  test_given_iterator_types<contiguous_iterator<const int*>, contiguous_iterator<const int*>>();
+}
----------------
in other tests, we usually do
```
template <class It1, class Int2>
void test();

template <class It2>
void testAllIterator1(){
  test<InputIteartor, It2>();
  test<ForwardIteartor, It2>();
  // ...
  test<ContiguousIteartor, It2>();
}

void testAllIt1It2(){
  testAllIterator1<InputIteartor>();
  testAllIterator1<ForwardIteartor>();
  // ...
  testAllIterator1<ContiguousIteartor>();
}
```

This saves you manually writing down the all the cartesian products.

Some people also argue that we only need to test the weakest iterator and the strongest iterator to save the combinatorial code bloat.  It also has a point but I am fine with testing all of the combinations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395