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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/min_element.h:49
 template <class _ForwardIterator>
 _LIBCPP_NODISCARD_EXT inline
 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
Mordante wrote:
> 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.
Yup, I omit it too. I will definitely share the results.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114136