[PATCH] D52697: Bug 39129: Speeding up partition_point/lower_bound/upper_bound/ by using unsigned division by 2 when possible.

[Eric Fiselier via Phabricator]

EricWF added a reviewer: EricWF.
EricWF added inline comments.


================
Comment at: include/algorithm:3199
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+    _Tp operator()(const _Tp& __value_) const {
+        return __value_ / 2;
----------------
`__value_` with the trailing underscore should only be used for members. And pass by value.

You'll have to forgive the existing code around you that's doing it wrong.


================
Comment at: include/algorithm:3205
+
+#if _LIBCPP_STD_VER >= 11  // There isn't an easy compile time way to get
+                           // numeric_limits::max() in C++03
----------------
This is always true. It's a quirk of `_LIBCPP_STD_VER` that it's never less than 11. What you want here is `#if !defined(_LIBCPP_CXX03_LANG)`


================
Comment at: include/algorithm:3210
+    typedef typename conditional<
+            std::numeric_limits<_Integral>::max() <= numeric_limits<size_t>::max(),
+            size_t,
----------------
If you really want to make this work in C++03, You should be able to use the super-secret member `numeric_limits<Integral>::__max`. But this only works because you know you're dealing with a builtin type in this specialization.


================
Comment at: include/algorithm:3223
+
+// Casts to size_t to divide the number in half (if the type conversion is safe).
+// Implicit assumption - the cast will happen from the positive number.
----------------
I think this comment can be reduced to something like:

```
// Perform division by two quickly for positive integers (llvm.org/PR39129)
```

We want to keep our header sizes small


================
Comment at: include/algorithm:3227
+// This is necessary because division by two is faster for unsigned numbers and it
+// matters (Bug 39129).
+//
----------------
s/Bug 39129/llvm.org/PR39129


================
Comment at: include/algorithm:3233
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+_Tp __half_as_unsigned(const _Tp& __value_) {
+    return __do_half_as_unsigned<_Tp>()(__value_);
----------------
s/`__value_`/`__value`. And I think we can pass by value here.


================
Comment at: include/algorithm:3233
+_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
+_Tp __half_as_unsigned(const _Tp& __value_) {
+    return __do_half_as_unsigned<_Tp>()(__value_);
----------------
EricWF wrote:
> s/`__value_`/`__value`. And I think we can pass by value here.
Nit on the name:

I would rather it reflected (A) that the number must be positive, and (B) it's fast. The fact that it does it using unsigned numbers is an implementation detail.


================
Comment at: include/algorithm:3245
     {
-        difference_type __l2 = __len / 2;
+        difference_type __l2 = __half_as_unsigned(__len);
         _ForwardIterator __m = __first;
----------------
Needs _VSTD:: qualifier to defeat ADL.


================
Comment at: include/algorithm:4105
+template <class _Compare, class _Tp, class _Reference>
+struct __less_than_t
+{
----------------
I would prefer `__less_than_t` and `__less_than` had more specific names which reflect how the types/functions bind to comparators/iterators.

That being said, I'm also OK if we scrap this optimization in C++03 and then implement it using C++11 lambdas.


================
Comment at: include/algorithm:4107
+{
+    _Compare __comp_;  // We can't use an empty base class optimization for comp
+                       // since it might be a function pointer.
----------------
This seems like a job for `__compressed_pair`. 


================
Comment at: include/algorithm:4110
+                       //
+                       // Because this functor is compiled out completly, it doesn't matter.
+    const _Tp* __value_;
----------------
There is no reason the functor is guaranteed to be compiled out completely, is there?


================
Comment at: include/algorithm:4115
+    __less_than_t(_Compare __comp, const _Tp& __value_) :
+        __comp_(__comp),
+        __value_(_VSTD::addressof(__value_)) {}
----------------
We could move here.


================
Comment at: include/algorithm:4235
     {
-        difference_type __l2 = __len / 2;
+        difference_type __l2 = __half_as_unsigned(__len);
         _ForwardIterator __m = __first;
----------------
Needs _VSTD:: qualifier to defeat ADL.


https://reviews.llvm.org/D52697