Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
We recently started implementing libc++ is more granular headers. Can you adopt that style for these changes too?
You an use D103734 as an example.
I see a lot of build failure. I did a quick look and I expect none of them are due to this patch, but to main not working properly. Can you rebase your patch to see whether it passes the build?
libcxx/include/compare | ||
---|---|---|
149 | Please update the synopsis. | |
libcxx/test/std/language.support/cmp/cmp.result/compare_three_way_result.pass.cpp | ||
46 ↗ | (On Diff #349433) | Please make these line fit in our maximum column width of 120 characters. |
76 ↗ | (On Diff #349433) | I'd like to see some more tests with fundamental and library types. |
libcxx/test/std/language.support/cmp/cmp.result/compare_three_way_result.pass.cpp | ||
---|---|---|
30 ↗ | (On Diff #349433) | friend std::strong_ordering operator<=>(const OtherOperand&, const OtherOperand&) = delete; but also consider whether it would break anything to make this simply friend std::strong_ordering operator<=>(OtherOperand, OtherOperand) = delete; because empty structs are typically passed by value. (Or even omit it entirely, although I assume that would break something you're trying to test here, is that right?) |
46 ↗ | (On Diff #349433) | Throughout the file, make these substitutions: ASSERT_SAME_TYPE(std::compare_three_way_result_t<OT&, OT&>, SameOrder>); which comfortably fits within our informally enforced column limit. |
Mark the test file as requiring Concepts support. (The Mac OSX buildkite runs were failing.)
Otherwise LGTM
libcxx/include/__compare/compare_three_way_result.h | ||
---|---|---|
28 | We can probably use that __add_lvalue_reference_t<_Tp> alias here. |
libcxx/include/__compare/compare_three_way_result.h | ||
---|---|---|
28 | Good point. Done. |
LGTM, but can you please add a test that std::is_same_v<compare_three_way_result<T>, compare_three_way_result<T, T>>?
We can probably use that __add_lvalue_reference_t<_Tp> alias here.