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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with comments. Thanks for doing this!



================
Comment at: libcxx/include/__algorithm/is_heap_until.h:50
+template <class _RandomAccessIterator, class _Compare>
+_LIBCPP_NODISCARD_EXT _LIBCPP_CONSTEXPR_AFTER_CXX17 _RandomAccessIterator
+is_heap_until(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare __comp)
----------------
We do need `_LIBCPP_HIDE_FROM_ABI` here and in other places.


================
Comment at: libcxx/include/__algorithm/min_element.h:49
 template <class _ForwardIterator>
 _LIBCPP_NODISCARD_EXT inline
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114136