[libcxx-commits] [PATCH] D98983: [libcxx] adds concepts `std::totally_ordered` and `std::totally_ordered_with`

Christopher Di Bella via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Apr 1 23:07:16 PDT 2021


cjdb marked 6 inline comments as done.
cjdb added a comment.

In D98983#2665813 <https://reviews.llvm.org/D98983#2665813>, @zoecarver wrote:

> (Also, please run this through clang-format or fix the formatting issues I pointed out earlier before you commit.)

This is the product of clang-format. Per @ldionne's musings in D99691 <https://reviews.llvm.org/D99691>, manually overriding clang-format is going to become a hard error in the future.

In D98983#2665810 <https://reviews.llvm.org/D98983#2665810>, @zoecarver wrote:

> Okay, this (finally) looks good to me. As we discussed, I think cutting down these tests a bit in the future will help me review it more quickly (and it would make me feel more confident that I haven't overlooked something). Maybe we should have a larger discussion about how we want to do concept tests in the discord channel. I think it would be beneficial to everyone to solidify some guidelines.

Yes, future patches will have a reduced set of tests for the most part, because they're not (a) testing the boundary of the language and library, and (b) testing a completely novel compiler feature with minimal field experience. D96477 <https://reviews.llvm.org/D96477> is an example of this plan.
Having said that, a discussion would be welcome to make sure everyone's on the same page.



================
Comment at: libcxx/test/std/concepts/concepts.compare/concepts.totallyordered/totally_ordered.pass.cpp:115
+struct A {};
+// FIXME(cjdb): uncomment when operator<=> is implemented for each of these types.
+// static_assert(!std::totally_ordered<std::array<A, 10> >);
----------------
zoecarver wrote:
> zoecarver wrote:
> > I think I brought this up before, but do you want to create a bug for spaceship support and then put a link to it in all the tests that need to be updated? That way you could just do a cmd+f to find all the places that mention the bug ID and need to be updated, rather than searching through all "FIXME" comments. 
> Just to be clear, I'm not asking you to do this. I'm just suggesting it, as you are likely the one who will be updating these tests ;)
It's a good idea, and I'd //love// to do that, but apparently that's not what Buganizer is used for (at least, according to @mclow.lists).


================
Comment at: libcxx/test/std/concepts/concepts.compare/types.h:13
 #include <concepts>
+#include <string>
 #include <type_traits>
----------------
zoecarver wrote:
> Why do we now need to include <string>?
Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98983



More information about the libcxx-commits mailing list