[libcxx-commits] [PATCH] D114136: [libc++] Test that our algorithms never copy a user-provided comparator.

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

Quuxplusone marked 2 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/is_heap_until.h:24
-_LIBCPP_NODISCARD_EXT _LIBCPP_CONSTEXPR_AFTER_CXX17 _RandomAccessIterator
-is_heap_until(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp)
 {
----------------
This was missing `_LIBCPP_HIDE_FROM_ABI`; I've added it.


================
Comment at: libcxx/include/__algorithm/is_sorted_until.h:24
-_LIBCPP_NODISCARD_EXT _LIBCPP_CONSTEXPR_AFTER_CXX17 _ForwardIterator
-is_sorted_until(_ForwardIterator __first, _ForwardIterator __last, _Compare __comp)
 {
----------------
This was missing `_LIBCPP_HIDE_FROM_ABI`; I've added it.


================
Comment at: libcxx/include/__algorithm/min_element.h:49
 template <class _ForwardIterator>
 _LIBCPP_NODISCARD_EXT inline
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
ldionne wrote:
> Quuxplusone wrote:
> > In a couple of these cases I drive-by removed `inline` from function templates. I could put it back.
> > Do we think `inline` has any beneficial effect, these days?
> > ```
> > $ git grep -L 'inline' ../libcxx/include/__algorithm/*.h
> > ../libcxx/include/__algorithm/comp.h
> > ../libcxx/include/__algorithm/half_positive.h
> > ../libcxx/include/__algorithm/is_partitioned.h
> > ../libcxx/include/__algorithm/partition_copy.h
> > ../libcxx/include/__algorithm/partition_point.h
> > ../libcxx/include/__algorithm/remove.h
> > ../libcxx/include/__algorithm/remove_if.h
> > ../libcxx/include/__algorithm/shuffle.h
> > ../libcxx/include/__algorithm/sift_down.h
> > ```
> I would avoid removing it in this PR. As it happens, I've been doing the same from time to time while refactoring code, and I currently have on my plate to investigate a not insignificant code size regression we've witnessed when building our shared linker cache against a recent libc++, which I suspect is due to that based on some initial investigation.
> 
> Before we know for sure what's going on, I would not touch `inline`.
Okay. In the new revision, I think I've added `inline` to all the places. That is:
- When splitting up an already-`inline` function (`min_element`) into two new functions, I keep `inline` on both of them.
- When splitting up a non-`inline` function (`is_heap_until`), I add `inline` to the smaller/public'er one and keep `inline` //off// of the larger/now-internal one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114136