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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge_comp.pass.cpp:9
 
-// Older compilers don't support std::is_constant_evaluated
-// UNSUPPORTED: clang-4, clang-5, clang-6, clang-7, clang-8
-// UNSUPPORTED: apple-clang-9, apple-clang-10
+// UNSUPPORTED: clang-8
 
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > What happened to all the other unsupported compilers? 
> `// UNSUPPORTED: clang-8` was good enough for ee95c7020cebe4668a501f24305a76a044df5266, so I expect it's also good enough here.
> 
> I strongly suspect that the test harness somehow knows that `UNSUPPORTED: clang-8` implies `UNSUPPORTED: clang-7` and so on.
> I have no particular explanation for what's the deal with `apple-clang-9, apple-clang-10`.
> Alternatively, it's possible that no buildbot runs `clang-7` anymore?
> Alternatively, it's possible that no buildbot runs clang-7 anymore?

That's it. The test harness does not do anything special w.r.t. clang versions. It couldn't, because a bug could exist in clang-7 but not clang-6, so it would be invalid to assume that the `clang-7` Lit feature implies the `clang-6` Lit feature, for example. The truth is that nobody cares about libc++ "working" with several of the compilers we claim to support. For example, I doubt anyone really builds libc++ with GCC 5, except perhaps in some fake CI setups -- but nobody actually ships that, and I'd be scared if they did.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/merge_comp.pass.cpp:44
+
+struct NonTrivial {
+    int value;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92255