Implements [variant.relops] and [variant.monostate.relops] for P1614R2
Details
- Reviewers
ldionne Mordante avogelsgesang - Group Reviewers
Restricted Project - Commits
- rGc4566cac490c: [libc++][spaceship] Implement std::variant::operator<=>
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please also update SpaceshipProjects.csv
libcxx/include/variant | ||
---|---|---|
1654–1655 | indentation here seems off using _ResultType = common_comparison_category_t<compare_three_way_result_t<_Types>...>; auto __three_way = []<class _Type>(const _Type& __v, const _Type& __w) -> ResultType { return __v <=> __w; }; | |
libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp | ||
56 | seems to be unused? |
- delete dead test function
- use a result type alias
- also implement std::monostate::operator<=>
- update spaceship projects
Updated SpaceshipProjects and added the missing monostate operator as well
libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp | ||
---|---|---|
56 | thanks! i discovered test_comparisons.h halfway through making this. |
libcxx/docs/Status/SpaceshipProjects.csv | ||
---|---|---|
27–28 | please also update monostate and variant to link to this review | |
libcxx/include/__variant/monostate.h | ||
34 | I usually try to keep the order between the synopsis and the implementation in sync. | |
libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp | ||
25 | afaik <variant> has a standard-mandated include to <compare. Hence, this include shouldn't be necessary |
libcxx/docs/Status/SpaceshipProjects.csv | ||
---|---|---|
27–28 | I updated the SpaceshipProjects.csv upstream to link a couple of in-flight reviews, also including this one. Please rebase on current main. | |
libcxx/include/__variant/monostate.h | ||
34 | Let's continue this discussion in https://reviews.llvm.org/D131363#inline-1263505 |
Thanks for working on this!
libcxx/include/__variant/monostate.h | ||
---|---|---|
33 | consexpr already implies inline. | |
libcxx/include/variant | ||
184–189 | we typically use remove in instead of until (like cppreference). | |
1645 | inline isn't needed here, both a template and constexpr imply that. | |
1649 | please use clang-format on this new code. | |
1655 | Why a separate lambda instead of a lambda in the visitor? | |
libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp | ||
25 | As a side note, you can just include headers unconditionally, they will be installed even when their contents in unavailable in a language version. (There are some exceptions, which apply when language features are disabled, like no localization available.) | |
28 | Can you improve this test and the next by using the comparison macros and using our current constexpr test pattern int main(int, const char**) { test(); static_assert(test()); return 0; } |
- add comment in test for tested operator
- don't conditionally include <compare>
- use runtime-and-static assert pattern for monostate test
- use test macros for monostate relops
libcxx/include/variant | ||
---|---|---|
1645 | Should inline therefore be removed from all of these operators? | |
1649 | just for this function? i can't seem to get my editor to run clang-format only on selected code, so it's quite painful. i'm also not sure if the result really helps readability here, it just shuffles some linebreaks around. are there specific formatting rules you'd like to be honored? | |
1655 | To keep line length and readability under control, mostly, same reason we added using for the return type. | |
libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp | ||
28 | done |
libcxx/include/variant | ||
---|---|---|
1645 | this whole file is only C++17 onward; i am less familiar with the progression of the associated keyword rules. should we have a separate differential to fix this if it's widespread within this file? |
libcxx/include/__variant/monostate.h | ||
---|---|---|
33 | as I learned today in a different review, _LIBCPP_INLINE_VISIBILITY was superseded by _LIBCPP_HIDE_FROM_ABI (see https://libcxx.llvm.org/DesignDocs/VisibilityMacros.html#visibility-macros) | |
libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp | ||
28 | please also update variant.relops/three_way.pass.cpp to follow this constexpr testing pattern and use the comparison test macros |
- modernize variant three_way test
- _LIBCPP_INLINE_VISIBILITY --> _LIBCPP_HIDE_FROM_ABI, remove unnecessary inlines
libcxx/include/__variant/monostate.h | ||
---|---|---|
33 | i've replaced all cases of _LIBCPP_INLINE_VISIBILITY with _LIBCPP_HIDE_FROM_ABI and removed inline in every case where the function itself was either constexpr or a template function. this establishes consistency and modern practice at the expense of ~90 lines of noise. let me know if we ought to fork that to a new differential. | |
libcxx/include/variant | ||
184–189 | i see 35 instances of "// until c++" and 41 instances of "// removed in c++" in v1/include. cppreference uses the two interchangeably, typically putting "until" next to the full signature (as we are writing here) and "removed in" next to the short name. (see: pair/operator_cmp, variant/operator_cmp, monostate). |
This is getting close to be ready, some minor issue remain.
libcxx/include/__variant/monostate.h | ||
---|---|---|
33 | Thanks! FYI I normally prefer those cleanups in a separate commit, but since they are simple changes I'm fine keeping them this time. | |
33 |
Historically we haven't always resolved our tech-debt, but we're better at it nowadays :-) | |
34 | This isn't something we really do in libc++, I don't object against it but I don't see it as mandatory. | |
libcxx/include/variant | ||
184–189 | I see, the code I looked at thus-far uses removed in. But feel free to keep this as is. | |
747 | please fix the formatting, can you also fix line 742? | |
1651 | The return is normally on its own line. FYI git clang-format HEAD^ can be used to format the changes in the last commit and then you can amend the wanted changes. This is basically what I do to format only relevant changes. | |
libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp | ||
19 | Can you move this to line 8? | |
74 | Why switch the order of V1 and V2? | |
107 | Can you add a test for std::partial_ordering::unordered? |
libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp | ||
---|---|---|
31–44 | testComparisons returns a bool indicating if it was succesful or not. The way you wrote this test case, it does not provide the intended test coverage. assert(testComparisons(m1, m2, /*isEqual*/ true, /*isLess*/ false)); Here and below in line 34 Also see https://reviews.llvm.org/D131364 and consider giving me a review on that |
- correctly wrap testOrder and testComparisons in asserts
- fix nits
- run git clang-format on all changes
- manually clang-format the added function
- manually fix spacing of multiline macros
- add partial_ordering::unordered test cases & utils support
- refactor testComparisons and clean up includes
- add nodiscard vis a vis D131364
- order things sensibly because this is C++
libcxx/include/variant | ||
---|---|---|
1651 | git clang-format HEAD^ does not seem to actually change anything in <variant> even when i squash all the changes in this differential into a single commit. it does clean up a couple things in some of the other files, though. likewise git clang-format main does nothing once those changes are applied; it never decides to act on <variant>. i let it format that function manually (even though i'm not really on board with un-braced controlled statements on a different line) and normalized all the indentation to the trailing backslashes in multiline macros to the old 80 column limit. but, i still don't know why it chose not to touch this file. | |
libcxx/test/std/utilities/variant/variant.monostate.relops/relops.pass.cpp | ||
31–44 | dang you're right, i saw that differential and didn't remember it here. it's a good change | |
libcxx/test/std/utilities/variant/variant.relops/three_way.pass.cpp | ||
19 | sure! | |
74 | great question, not sure how that happened | |
107 | yes! doing so revealed a core deficiency in the test_comparisons.h helper header as well, so i expanded upon that file to support testing unordered values. |
libcxx/test/support/test_comparisons.h | ||
---|---|---|
97 | copying @philnik's comment from https://reviews.llvm.org/D131364:
|
- assert return value types and simplify order value expressions
- add TEST_NODISCARD macro
- use TEST_NODISCARD
- formatting
libcxx/test/support/test_comparisons.h | ||
---|---|---|
97 | done. feel free to borrow the macro definition i made here, this should cover just about every case we care about, and i think it's right. |
libcxx/test/support/test_macros.h | ||
---|---|---|
238–246 ↗ | (On Diff #451963) | I think this should just be #if __has_cpp_attribute(nodiscard) # define TEST_NODISCARD [[nodiscard]] #else # define TEST_NODISCARD #endif I don't really see a reason to use warn_unused_result, since both clang and GCC have [[nodiscard]] as an extension since C++11 and Microsoft only tests with C++14 and later anyways AFAICT. Also, our __config header says: // We can't use GCC's [[gnu::warn_unused_result]] and // __attribute__((warn_unused_result)), because GCC does not silence them via // (void) cast. |
LGTM, after addressing @philnik's comment regarding the TEST_NODISCARD macro.
Thank you! :)
- simplify TEST_NODISCARD definition per feedback
libcxx/test/support/test_macros.h | ||
---|---|---|
238–246 ↗ | (On Diff #451963) | thanks! done |
libcxx/test/support/test_comparisons.h | ||
---|---|---|
154 | Likewise here, I feel that this function should also be asserting the same type is returned when the operands are swapped. Unless there's something I'm forgetting and that can't possibly happen due to how operators are resolved in c++20... | |
159–162 | Here's a question for you all: Should these instead be spelled as comparisons against zero? It seems possibly better not to assume the ordering type has normally named members, right? This seems like it would enable testing against user-typed orderings while assuming nothing about them except the basis of their usage in expressions. I also have been wondering if this function should assert the returned type of t1 <=> t2 (and probably t2 <=> t1); other than a recently created test in D130295 this already holds true in the entire test suite, and it seems like an unexpected gotcha that it actually doesn't assert anything about the type. This would especially hold true after the above change, when you could otherwise simply use int for the Order type (as long as we are not also checking (t2 <=> t1 == (0 <=> order)), which would not work with int). | |
163 | Furthermore, should this not also check that (t2 <=> t1 == (0 <=> order))? (and, depending on consensus above, check the type of t2 <=> t1) |
- re-add possibly less confusing assert to testComparisons that does not reference a parameter it doesn't have
- Merge branch 'main' into variant
- add sfinae tests for variant::operator<=>
- variant::operator<=> does NOT require three_way_comparable for its types in the standard
i suspect we can ship this differential as-is without fiddling with test_comparisons.h any further, and any further changes we want to make to that file should be done in another differential.
libcxx/include/variant | ||
---|---|---|
1645–1665 | it turns out this three_way_comparable concept requirement is far too strict and does not exist in the standard. all this operator needs to do is visit its children's operator<=>s, which is already performed by the return type. |
Thanks for your comments.
libcxx/test/support/test_comparisons.h | ||
---|---|---|
154 | Good point, feel free to add it. | |
159–162 | The types themselves are normally tested in the static_assert for the return type. So I don't think we need to do it here too. But it seems we didn't do it properly in D130295 so maybe it's useful. I had a look at the Standard, but is it allowed to return something else as one of the ordering categories? | |
163 | How do you feel about making a separate patch with the proposed improvements? |
libcxx/test/support/test_comparisons.h | ||
---|---|---|
159–162 | i think it makes sense to put it in here, since it seems like a gotcha that the returned order was actually different than the one you provided in the test. it seems trivially true that you should always provide the exact type that you expect to see whenever you call testOrder, and even if we assert the type there we still don't obsolete AssertOrderReturn since we aren't asserting anything about the basic operators returning bool. as for non-standard ordering types: it's an allowable thing to do because the usages of ordering-typed values are written in terms of order @ 0, as are the synthesized operations (for example, when a type only defines <=> and maybe ==). i know that in most places in the standard library, a lot of the things that are doing <=> need to return a common comparison category; user ordering types will never mesh with those so they are eliminated by SFINAE, but user orderings *do* work for synthesis of basic comparison operators, and indeed synth-three-way. if there is a standard function that returns a templated type's operator<=> result without gatekeeping on three_way_comparable_with, we might return a user ordering there (compare_three_way_result_t will resolve user ordering types). i have no idea whether any such cases exist. in the case of basic_string_view as one example the standard seems to explicitly state that non-standard ordering types are ill-formed. whatever the case for the code under test, it still seems like we can broaden the use-cases for this function by spelling things differently. just because we might not have a use for it in testing the behavior of actual standard functions doesn't mean we can't use it, like sanity checking the behavior of a type that returns custom orderings for testing purposes. | |
163 | yeah at this point i'm planning on doing this :) i can perhaps also try to tackle improving support/make_string.h to use char_traits correctly in the basic_string_view tests if you don't get there first. |
okay, every comment is either resolved or involves work that should go into another differential.
libcxx/include/variant | ||
---|---|---|
747 | i got these a couple updates ago :) |
LGTM, some comments but as discussed these should be done in separate patches.
libcxx/include/variant | ||
---|---|---|
747 | When I wrote the comment it wasn't done, I guess it wasn't marked as done before ;-) | |
libcxx/test/support/test_comparisons.h | ||
159–162 | Then let's add it as you suggested. However in that patch it would be good to refer to the Standard wording where this is allowed. This seems quite odd and confusing. (Just like equal and equivalent being the same and path having an equal and equivalent which isn't the same.) | |
163 | That would be great! I happily leave it to you; I have enough other things I want to work one ;-) |
please also update monostate and variant to link to this review