[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:45-47
+    ptrdiff_t __len1    = __last1 - __first1;
+    ptrdiff_t __len2    = __last2 - __first2;
+    ptrdiff_t __min_len = std::min(__len1, __len2);
----------------
var-const wrote:
> avogelsgesang wrote:
> > philnik wrote:
> > > (Thanks to @Quuxplusone for pointing it out!) Using `ptrdiff_t` is actually a bug because it might be smaller than the largest signed integer. Probably the easiest thing to do is to use `auto` instead and replace the `std::min()` with a ternary. For `__i` you could maybe use `decltype(__min_len)`. There might be a better way to do all this, I'm not sure.
> > Added a benchmark now. It shows a 30% improvement with the fast path
> As noted in another comment, can you please rerun the benchmark so that it compares `int*` with optimizations vs. `int*` without optimizations?
yes, rerunning the benchmark is definitely still on my todo list. It takes longer than expected, because it seems that one of the refactorings during this review destroyed the optimization. I can still reproduce the numbers on the old commit, but on the current review the fast path is no longer faster than the default path. I will need some time to figure out what exactly lead to the regression here...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131395