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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.

LGTM when the CI  is green.



================
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:
> 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.
> 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`.

@ldionne Interesting. Once you've finished your investigation can you share the results. I tend to omit `inline` for templates since it's not required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114136