[libc++] [P1222] std::flat_set

[Quuxplusone]

After this:

• Should flat_multiset be done in the same file, or should we granularize this into <__flat_set/flat_set.h> and <__flat_set/flat_multiset.h> before taking the next step? I tentatively think "two files," but only because that would make it easier to ensure things like "the identifier flat_set doesn't accidentally appear anywhere in flat_multiset.h nor vice versa." We don't expect any future standard header ever to include either of the granular headers alone.
• If we introduce __flat_set/, should sorted_unique.h and sorted_equivalent.h move into that directory? I tentatively think "yes," but I wouldn't like to bet on these types' never being reused by another STL facility. Compare __utility/in_place.h (used only in <any>, <expected>, <optional>, <ranges>, <variant>) but __variant/monostate.h (used only in <variant>).

[ldionne]

The complexity requirement here says that it should be O(N) if the container is already sorted. I don't think our std::stable_sort implementation has that property. I also agree that it would suck to call std::is_sorted. IMO that's worth discussing w/ LWG.

[ldionne]

Please add this to your list of issues to discuss in LWG. This shouldn't be implementation-defined. Code becomes super brittle when users start relying on that and it's not actually guaranteed. We even have a mode of libc++ that will purposefully shuffle stuff before sorting to prevent this sort of dependency from creeping up since the Google folks were being bitten by it.

[ldionne]

Please #if 0 this until the LWG issue is discussed and agreed upon, with a comment about the LWG issue.

[Quuxplusone]

This is https://wg21.link/lwg3802 , and the fix is already done as a separate commit with "LWG3802" in the subject line.
The code for LWG3802 is unconditionally missing prior to the fix, and unconditionally present after the fix.
The tests for LWG3802 are #if 0'ed out prior to the fix, and unconditionally enabled after the fix.

If libc++ doesn't want to merge the fix for LWG3802 until [some-unspecified-point-in-the-future], that's fine; you can just omit that specific git commit. But I'm going to leave each LWG-issue-fixing commit in this actual PR branch for now, because that makes it easier to review the intended code/tests. (Otherwise you'll just ask over and over "Why is this part of the code wrong?" and the answer will always be "Because it doesn't become correct until after LWG issue such-and-such." It doesn't make sense to review wrong code.)

[ldionne]

Same, pending LWG issue.

[Quuxplusone]

https://wg21.link/lwg3803 , fixed in a separate commit with "LWG3803" in the subject line.

[ldionne]

I think that would be a nice place to add a _LIBCPP_DEBUG_ASSERT(is_sorted). In fact, please make sure that you have a _LIBCPP_ASSERT for each precondition in this class. When the preconditions are expensive to check, we'll hide them behind the debug mode.

[ldionne]

Oh, and let's add a libc++ specific test for all of those, they are easy to write.

[ldionne]

I think we can actually use the hint. If the user gives us an iterator to the lower bound, then we can avoid computing it ourselves and perform the insertion directly at that location. You might be able to implement emplace in terms of emplace_hint in that case.

[Quuxplusone]

Done. I factored out an __emplace_unchecked(pos, K2&& x) that assumes the insertion does happen, at the exact location passed to it; all the insert/emplace sites use that now.

FWIW, I see that back in 2019 @tzlaine and I discussed this exact API (on the cpplang Slack) and Zach was at least briefly considering not to provide it at all. His reasoning was:

• mySet.insert(x) is O(lg n), but mySet.insert(pos, x) with the right hint is O(1).
• myFlatSet.insert(x) is O(n), but myFlatSet.insert(pos, x) with the right hint is... still O(n).

Does that change your calculus at all? (Of course at this point I've already done the implementation... but it's still a fair bit of code to maintain and test, and I admit I haven't written any good tests for all the emplace_hint corner cases yet. Ignoring the hint would be slightly less code and way fewer obscure branches to test.)

[ldionne]

I think I like the result. Let's keep using the hint, and let's make sure we have enough tests for it.

[ldionne]

All the places where you use stable_sort need the resolution on the issue we discussed yesterday.

[ldionne]

As you said, I think you can do better here: you can remove duplicates as you're doing inplace_merge. That would be something like a funky inplace_set_union where the right hand side is potentially a multiset. Maybe that's too messy and not worth doing.

[ldionne]

Can you add a comment explaining what this function does? Something like:

// Emplace the given element in the underlying container at the specified position.
// The position is assumed to be the correct position for that element, i.e. the underlying container
// should remain sorted and unique after calling this function.

[ldionne]

Suggested change

	if (__pos == begin() || __compare_(__pos[-1], __x)) {
	if (__pos == begin() || __compare_(*(__pos-1), __x)) {

This seems less clever to me.

[ldionne]

I think I like the result. Let's keep using the hint, and let's make sure we have enough tests for it.

[ldionne]

It's really unfortunate that we have to do a second binary search here to avoid making this O(N). I suspect that was not intended in the design. Let's note it down.

[ldionne]

Note to self: I haven't looked at CTAD yet since the current draft is missing some LWG issues resolutions.