[libcxx-commits] [PATCH] D92255: [libc++] [P0202] constexpr set_union, set_difference, set_symmetric_difference, merge

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 5 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge_comp.pass.cpp:44
+
+struct NonTrivial {
+    int value;
----------------
ldionne wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > zoecarver wrote:
> > > > Can we put these all into a header?
> > > We could, but they're small enough that I think that could wait until we want to use them elsewhere.
> > > 
> > > Also, note that the classes in the comparator tests are deliberately //not// `<`-comparable, just to verify that we never accidentally try to use their `operator<`. So we really have 4 different classes here, not just 2.
> > > 
> > > Background on my reluctance here: In the process of writing this PR I discovered `TrackedValue`, which @ldionne wrote back in 2015, put in a header, and then never used again. (I wanted to make `NonTrivial` just a `pair<Trivial, TrackedValue>` or something like that; but it turned out that `TrackedValue` wasn't constexpr, so I couldn't use it anyway.)
> > It looks like this is used in 10 different places. Yes, it's a small class so maybe we don't want to put it in a header for that reason, but if there's a bug that we need to fix later or something needs to be changed, I'd rather update one header than 10 different test files. Additionally, if there is a bug that is only revealed in one of the tests, the other nine classes won't be updated. @ldionne what do you think?
> I think I would put them in a header e.g. in `test/std/algorithms/alg.sorting/` so that they are shared across these tests. Lifting the utilities all the way into `test/support` might not make sense, though, since they are kinda specific to the sort of tests you're trying to write here.
> 
> @Quuxplusone  @zoecarver  Would that make sense to you both?
I still think it's //unnecessary//, but I don't //object//. Of course, the naming part is hard. I propose (and will update the PR with, unless someone stops me) `"alg.sorting/sortable_helpers.h"` containing `TrivialSortable`/`NonTrivialSortable` and `TrivialSortableWithComp`/`NonTrivialSortableWithComp`.

I still feel moderately strongly that `TrivialSortableWithComp`/`NonTrivialSortableWithComp` should //not// have a well-formed `operator<`, and so must remain a different pair of classes from the former two. It'd be easy enough to give the `T::less` comparator some different behavior from `T::operator<`, so that if the algo accidentally used the wrong one, the test would detect that bug at runtime; //but//, it wouldn't be able to detect a bug in which the algo was wrongly constrained to require `operator<`, unless we actually instantiate the algo with a non-`operator<`-comparable type.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge_comp.pass.cpp:61
+#if TEST_STD_VER >= 11
+static_assert(std::is_trivially_copyable<Trivial>::value, "");
+static_assert(!std::is_trivially_copyable<NonTrivial>::value, "");
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > I think these will work in C++03. 
> > Officially, neither `static_assert` nor `is_trivially_copyable` are present in C++11. `test/libcxx/containers/sequences/vector.bool/trivial_for_purposes_of_call.pass.cpp` is unsupported in C++03 and I can't see any reason it would be unsupported //except// for its reliance on the `is_trivially_` type-traits.
> libc++ provides many of these things in C++03. Even rvalues and variadics work.
I'll update the patch and see what buildkite thinks of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92255