[libcxx-commits] [PATCH] D113868: [libcxx] Cast to the right `difference_type` in various algorithms

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

LGTM mod the one unnecessary diff, and indentation. (Don't trust clang-format; it likes to mess up libc++ style.)



================
Comment at: libcxx/include/__algorithm/copy_n.h:60
 {
-    typedef decltype(_VSTD::__convert_to_integral(__orig_n)) _IntegralSize;
-    _IntegralSize __n = __orig_n;
-    return _VSTD::copy(__first, __first + __n, __result);
+  typedef typename iterator_traits<_InputIterator>::difference_type difference_type;
+  typedef decltype(_VSTD::__convert_to_integral(__orig_n)) _IntegralSize;
----------------
Note for the record: This un-uglified name is technically OK because it's a library reserved name. We actually do the same trick in `__make_heap` below. If I hadn't seen that in `__make_heap`, I'd totally have objected to this. ;) But given the precedent, I think it's OK.


================
Comment at: libcxx/include/__algorithm/make_heap.h:33
         // start from the first parent, there is no need to consider children
-        for (difference_type __start = (__n - 2) / 2; __start >= 0; --__start)
-        {
-            _VSTD::__sift_down<_Compare>(__first, __last, __comp, __n, __first + __start);
+        for (difference_type __start = (__n - difference_type(2)) / 2; __start >= 0; --__start) {
+          _VSTD::__sift_down<_Compare>(__first, __last, __comp, __n, __first + __start);
----------------
We shouldn't //need// to cast the `2` here — this is just plain old "integral minus integral" subtraction. We don't cast `1` on line 30, for example. So I recommend reverting this diff.


================
Comment at: libcxx/include/__algorithm/make_heap.h:34
+        for (difference_type __start = (__n - difference_type(2)) / 2; __start >= 0; --__start) {
+          _VSTD::__sift_down<_Compare>(__first, __last, __comp, __n, __first + __start);
         }
----------------
You've broken the indentation here (and various places, especially in `sort.h`). Please make sure to preserve the existing indentation style wherever you change code. (If it's 2-space, leave it 2-space; if it's 4-space, leave it 4-space; don't mix styles within the same function.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113868