This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Test that our algorithms never copy a user-provided comparator.
ClosedPublic

Authored by Quuxplusone on Nov 17 2021, 9:21 PM.

Details

Summary

This is not mandated by the standard, so it goes in libcxx/test/libcxx/.
It's certainly arguable that the algorithms changed here (is_heap, is_sorted, min, max) are harmless and we should just let them copy their comparators once. But at the same time, it's nice to have all our algorithms be 100% consistent and never copy a comparator, not even once.

P.S. — If I find the time, I'll investigate the thing I was talking about way back in D93562, where we change _Comp_ref to be a lightweight value type instead of a reference type, and then we never need to pass the explicit argument <_Comp_ref> to anything. That would be a vastly more invasive PR, touching all the algorithms; this new test would be the regression-test that we would expect to continue passing even after that invasive change.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Nov 17 2021, 9:21 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
libcxx/include/__algorithm/min_element.h
38–39

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
libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp
105

This one is different because for_each returns the comparator by move (and for UnaryVoid a move is a copy).

ldionne accepted this revision.Nov 18 2021, 6:23 AM

LGTM with comments. Thanks for doing this!

libcxx/include/__algorithm/is_heap_until.h
51

We do need _LIBCPP_HIDE_FROM_ABI here and in other places.

libcxx/include/__algorithm/min_element.h
38–39

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.

This revision is now accepted and ready to land.Nov 18 2021, 6:23 AM
ldionne added inline comments.Nov 18 2021, 6:24 AM
libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp
121

When/where was TEST_IS_CONSTANT_EVALUATED introduced? I seem to have missed it, but it certainly looks weird to me.

libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp
121

It came originally from D110598 (at my instigation), but ended up landing yesterday in D113906.
I already know that within 3 years we'll need something finer-grained; I suggested

#define TEST_IS_RUNTIME_OR_CXX20 ...  // for guarding code that became constexpr-friendly in '20
#define TEST_IS_RUNTIME_OR_CXX23 ...  // for guarding code that became constexpr-friendly in '23

But it'll be easy to search-and-replace this later, as soon as someone needs it, so I figure there's no rush to settle on that prematurely.

Quuxplusone marked 2 inline comments as done.Nov 18 2021, 7:57 AM
Quuxplusone added inline comments.
libcxx/include/__algorithm/is_heap_until.h
24

This was missing _LIBCPP_HIDE_FROM_ABI; I've added it.

libcxx/include/__algorithm/is_sorted_until.h
24

This was missing _LIBCPP_HIDE_FROM_ABI; I've added it.

libcxx/include/__algorithm/min_element.h
38–39

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.

Don't remove inline; do include-what-you-use __comp_ref_type. CI should be happy now.

nilayvaish added inline comments.Nov 18 2021, 8:29 AM
libcxx/include/__algorithm/is_heap_until.h
52

Why does this need to be hidden? Also isn't this already part of the ABI?

nilayvaish accepted this revision.Nov 18 2021, 8:38 AM
nilayvaish added inline comments.
libcxx/include/__algorithm/is_heap_until.h
52

Found the ABI spec here: https://libcxxabi.llvm.org/spec.html. So this seems good.

ldionne added inline comments.Nov 18 2021, 8:49 AM
libcxx/include/__algorithm/is_heap_until.h
52

Just to be clear, that document only explains the functions exported by libc++abi as an implementation of the Itanium C++ ABI.

The reason for hiding is_heap_until is that we don't want it to be part of libc++'s ABI surface in any way, so we can make changes to it later.

Oops, fix a copy-paste typo. Now CI should be happy!

Mordante accepted this revision.Nov 18 2021, 10:53 AM

LGTM when the CI is green.

libcxx/include/__algorithm/min_element.h
38–39

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.

ldionne added inline comments.Nov 18 2021, 12:42 PM
libcxx/include/__algorithm/min_element.h
38–39

Yup, I omit it too. I will definitely share the results.

Aaargh. Fourth time's the charm.
(Also, GCC was complaining about the one-line if (!TEST_IS_CONSTEXPR) (void)foo(); assert(bar); so I added curly braces to them.)

var-const added inline comments.Nov 18 2021, 2:56 PM
libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp
10

Same comment re. <numeric> as the other patch.

17

Optional: consider using a mixin to reduce boilerplate:

struct CopyTracker {
  int *copies_;
  TEST_CONSTEXPR explicit CopyTracker(int *copies) : copies_(copies) {}
  TEST_CONSTEXPR CopyTracker(const CopyTracker& rhs) : copies_(rhs.copies_) { *copies_ += 1; }
};

struct Less : CopyTracker {
  TEST_CONSTEXPR explicit Less(int *copies) : CopyTracker(copies) {}
  TEST_CONSTEXPR bool operator()(void*, void*) const { return false; }
};

// ...
171

Nit: I think LLVM style guide is 80 columns. Also, this would make the assert unconditional (applies below as well).

jloser added inline comments.Nov 18 2021, 5:35 PM
libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp
54

Are you using decltype(nullptr) instead of nullptr_t because of C++03 support or something else?

171

libc++ has its own .clang-format with a ColumnLimit of 120 FWIW.

Quuxplusone marked 5 inline comments as done.Nov 19 2021, 9:29 AM
Quuxplusone added inline comments.
libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp
10

Ack. Same answer: that it's a good idea and I should do it for all these (now three) tests but in a different PR.

17

I find "minimize layers of abstraction" to be the best rule for test code. We want to make sure that when it goes wrong, it goes wrong for the right reasons, and not [slight caricature here] because we messed up some template metaprogramming somewhere in the middle of the test. :) It's kinda the same reason I'm not a big fan of reusable helper headers.

54

Out of a completely mistaken set of beliefs that (1) Clang's C++03 mode supported decltype as an extension, (2) Clang's C++03 mode supported nullptr as an extension, (3) libc++'s C++03 mode did not define std::nullptr_t.
It turns out that Clang does not support decltype and nullptr in C++03; those are macros provided by libc++! And libc++ does provide std::nullptr_t. So I'll just switch to that. Thanks.

(In my own code I actually do use decltype(nullptr) out of laziness, because it's core-language and I can't be arsed to remember which header defines the library alias std::nullptr_t. But in this patch-series Louis has already warned me against laziness. ;))

171

Unconditional assert detected by GCC buildkite, and fixed; thanks.
Line-length ugly, but doubling the number of lines strikes me as strictly uglier, so I'm calling this fine.

Quuxplusone marked 4 inline comments as done.

Fifth time's the charm? Buildkite hates me. :)
Removed decltype(nullptr) in favor of std::nullptr_t; fixed up some constexpr-friendliness.

var-const added inline comments.Nov 19 2021, 8:19 PM
libcxx/test/libcxx/algorithms/robust_against_copying_comparators.pass.cpp
171

libc++ has its own .clang-format with a ColumnLimit of 120 FWIW.

Thanks!