[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Oct 28 10:47:11 PDT 2022
ldionne requested changes to this revision.
ldionne added a subscriber: jwakely.
ldionne added a comment.
This revision now requires changes to proceed.
We're getting there! We stopped right before the `void` specialization today.
================
Comment at: libcxx/include/__expected/expected.h:104
+ is_constructible<_Err, _Gf>, //
+ _Not<is_constructible<_Tp, expected<_Up, _Gp>&> >, //
+ _Not<is_constructible<_Tp, expected<_Up, _Gp>> >, //
----------------
Nitpick but you don't need those.
================
Comment at: libcxx/include/__expected/expected.h:108
+ _Not<is_constructible<_Tp, const expected<_Up, _Gp>> >, //
+ _Not<is_convertible<expected<_Up, _Gp>&, _Tp> >, //
+ _Not<is_convertible<expected<_Up, _Gp>&&, _Tp> >, //
----------------
I do not understand why the spec checks for `!is_convertible_v<expected<U, G>&, T>` after having already checked for `!is_constructible_v<T, expected<U, G>&>`. Shouldn't be `!is_constructible` imply `!is_convertible`?
The relevant sections are http://eel.is/c++draft/expected#object.cons-17.3 and http://eel.is/c++draft/expected#object.cons-17.7.
@jwakely Since you co-authored this paper, do you have any insight for me here? I assume I need to be taught something about C++, cause I'm certain there is a reason for this constraint.
Edit: @huixie90 just came up with an example where there's a distinction (which involves an ambiguous constructor call but non-ambiguous conversion), but I'm still curious to hear what's your take on this.
================
Comment at: libcxx/include/__expected/expected.h:119
+ template <class _Gf>
+ static constexpr bool __can_assign_from_unexpected = _And< //
+ is_constructible<_Err, _Gf>, //
----------------
And this too, closer to the assignments.
================
Comment at: libcxx/include/__expected/expected.h:129
+ template <class _T1, class _T2, class... _Args>
+ _LIBCPP_HIDE_FROM_ABI static constexpr void __reinit_expected(_T1& __newval, _T2& __oldval, _Args&&... __args) {
+ if constexpr (is_nothrow_constructible_v<_T1, _Args...>) {
----------------
Can we move this closer to the assignment operators?
================
Comment at: libcxx/include/__expected/expected.h:137
+ std::construct_at(std::addressof(__newval), std::move(__tmp));
+ } else {
+ _T2 __tmp(std::move(__oldval));
----------------
I think we should make the precondition that `is_nothrow_move_constructible<_T2>` explicit here by adding a `static_assert`. Failure to comply to that, if it happened, would lead to incredibly tricky runtime errors.
You could also add a short comment explaining what this does, and that it provides the strong exception guarantee.
================
Comment at: libcxx/include/__expected/expected.h:146
+
+ _LIBCPP_HIDE_FROM_ABI static constexpr void __swap_val_unex_impl(expected& __with_val, expected& __with_err) //
+ noexcept(is_nothrow_move_constructible_v<_Tp>&& is_nothrow_move_constructible_v<_Err>) {
----------------
Closer to `swap`?
================
Comment at: libcxx/include/__expected/expected.h:147
+ _LIBCPP_HIDE_FROM_ABI static constexpr void __swap_val_unex_impl(expected& __with_val, expected& __with_err) //
+ noexcept(is_nothrow_move_constructible_v<_Tp>&& is_nothrow_move_constructible_v<_Err>) {
+ if constexpr (is_nothrow_move_constructible_v<_Err>) {
----------------
You seem to have this in other places, consider doing a simple grep for `>&&` and fixing places that are forgotten whitespace.
================
Comment at: libcxx/include/__expected/expected.h:152-155
+ std::construct_at(std::addressof(__with_err.__val_), std::move(__with_val.__val_));
+ std::destroy_at(std::addressof(__with_val.__val_));
+ std::construct_at(std::addressof(__with_val.__unex_), std::move(__tmp));
+ __trans.__complete();
----------------
I think I would do this instead. Simply because the two other lines can't possibly throw (otherwise we have other big big problems).
================
Comment at: libcxx/include/__expected/expected.h:157
+ } else {
+ _Tp __tmp(std::move(__with_val.__val_));
+ std::destroy_at(std::addressof(__with_val.__val_));
----------------
Let's `static_assert` that `T` is nothrow-move-constructible, since we rely on it.
================
Comment at: libcxx/include/__expected/expected.h:160-163
+ std::construct_at(std::addressof(__with_val.__unex_), std::move(__with_err.__unex_));
+ std::destroy_at(std::addressof(__with_err.__unex_));
+ std::construct_at(std::addressof(__with_err.__val_), std::move(__tmp));
+ __trans.__complete();
----------------
Same reason.
================
Comment at: libcxx/include/__expected/expected.h:215
+ _LIBCPP_HIDE_FROM_ABI constexpr expected(expected&& __other) //
+ noexcept(is_nothrow_move_constructible_v<_Tp>&& //
+ is_nothrow_move_constructible_v<_Err>)
----------------
================
Comment at: libcxx/include/__expected/expected.h:234
+ !is_convertible_v<const _Gp&, _Err>) //
+ expected(const expected<_Up, _Gp>& __other) //
+ noexcept(is_nothrow_constructible_v<_Tp, const _Up&>&& //
----------------
I would suggest not indenting the function name at the same level as `explicit` and `noexcept`. It took me a few seconds to figure out which of these lines was the function name and parameter lists.
================
Comment at: libcxx/include/__expected/expected.h:261
+
+ template <class _Up = _Tp>
+ requires(!is_same_v<remove_cvref_t<_Up>, in_place_t> && //
----------------
Are you testing this default argument? IIRC, this was to support initializer lists being used, e.g. `expected({args...})`, and without the default argument it wouldn't be able to deduce anything. IIRC this was added by some LWG issue but I am failing to find it right now.
================
Comment at: libcxx/include/__expected/expected.h:310
+
+# if __cpp_concepts >= 202002
+ _LIBCPP_HIDE_FROM_ABI constexpr ~expected()
----------------
Which compiler(s) do not satisfy that?
================
Comment at: libcxx/include/__expected/expected.h:358
+ }
+ __has_val_ = __rhs.__has_val_;
+ return *this;
----------------
I think this comment (or a variant of it) is useful to understand what's going on.
================
Comment at: libcxx/include/__expected/expected.h:383
+ }
+ __has_val_ = __rhs.__has_val_;
+ return *this;
----------------
Same comment about strong exception guarantee.
================
Comment at: libcxx/include/__expected/expected.h:387
+
+ template <class _Up = _Tp>
+ _LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(_Up&& __v)
----------------
Same comment for testing this behavior!
================
Comment at: libcxx/include/__expected/expected.h:422
+ if (has_value()) {
+ __reinit_expected(__unex_, __val_, std::move(__un.error()));
+ __has_val_ = false;
----------------
Here you use `std::move(__un.error())` but above you used `std::move(__un).error()` -- both work, and I don't mind which one we use, but I think we should be consistent. FWIW the spec uses `std::move(__un.error())`, so we might want to stick with that and change the other one.
================
Comment at: libcxx/include/__expected/expected.h:491
+ _LIBCPP_HIDE_FROM_ABI constexpr const _Tp* operator->() const noexcept {
+ _LIBCPP_ASSERT(__has_val_, "expected must contain a value");
+ return std::addressof(__val_);
----------------
A bit more descriptive, and this can also be used to ensure that the right code is being triggered by `assert.xxxxxx` tests.
================
Comment at: libcxx/include/__expected/expected.h:574
+ _LIBCPP_HIDE_FROM_ABI constexpr _Tp value_or(_Up&& __v) const& {
+ static_assert(is_copy_constructible_v<_Tp>, "value_type has to be copy constructible");
+ static_assert(is_convertible_v<_Up, _Tp>, "argument has to be convertible to value_type");
----------------
Can we please test these "Mandates" diagnostics (and below)? You can use a `.verify.cpp` test and ensure that the right diagnostic is produced.
================
Comment at: libcxx/include/__expected/expected.h:612-616
+ bool __has_val_;
+ union {
+ _Tp __val_;
+ _Err __unex_;
+ };
----------------
I would like to suggest swapping the union and the `bool` here. I think that will result in a more space efficient layout in most cases. You could confirm and change, or infirm and keep.
================
Comment at: libcxx/include/__expected/expected.h:75-89
+ static constexpr bool __can_convert =
+ is_constructible_v<_Tp, _Uf> && //
+ is_constructible_v<_Err, _Gf> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>> && //
----------------
huixie90 wrote:
> philnik wrote:
> > ldionne wrote:
> > > That will evaluate these `is_constructible<...>` expressions in a short-circuiting fashion.
> > What exactly is the benefit of short-circuiting here? Is it required to satisfy the spec? If not, are you sure it's worth instantiating types and short-circuiting instead of using variables? I think instantiating types takes quite a bit longer, but I might be wrong on that.
> This is a good question. `is_xxx_v<T>` is no longer `is_xxx<T>::value` now. It is directly calling the compiler builtin. @ldionne , what do you think?
>
> Btw, I did a simple test with clang to compare the compile times between different approaches
>
> Program 1
> ```
> #include <type_traits>
> #include <utility>
>
> constexpr std::size_t template_length = 30000;
> template <std::size_t N> struct Foo {
> constexpr Foo(int)
> requires(N < template_length / 2)
> {}
> };
>
> template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> static_assert((std::is_constructible_v<Foo<Ns>, int> && ...));
> };
>
> template <std::size_t N> void generateTest() {
> test(std::make_index_sequence<N>{});
> }
>
> int main() { generateTest<template_length>(); }
>
> ```
> `time clang++ a.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.00secs`
>
> Program 2
> ```
> #include <type_traits>
> #include <utility>
>
> constexpr std::size_t template_length = 30000;
> template <std::size_t N> struct Foo {
> constexpr Foo(int)
> requires(N < template_length / 2)
> {}
> };
>
> template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> static_assert((std::is_constructible<Foo<Ns>, int>::value && ...));
> };
>
> template <std::size_t N> void generateTest() {
> test(std::make_index_sequence<N>{});
> }
>
> int main() { generateTest<template_length>(); }
>
> ```
> `time clang++ b.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.04secs`
>
> Program 3
> ```
> #include <type_traits>
> #include <utility>
>
> constexpr std::size_t template_length = 30000;
> template <std::size_t N> struct Foo {
> constexpr Foo(int)
> requires(N < template_length / 2)
> {}
> };
>
> template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> static_assert((std::_And<std::is_constructible<Foo<Ns>, int>...>::value));
> };
>
> template <std::size_t N> void generateTest() {
> test(std::make_index_sequence<N>{});
> }
>
> int main() { generateTest<template_length>(); }
>
> ```
> `time clang++ c.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `1.44 secs`
>
> @philnik So for this simple program, short circuiting (at the half position) is helpful
> What exactly is the benefit of short-circuiting here?
The benefit is that we avoid doing work (defined after) if we can. The work I'm thinking about is: instantiating types and/or aliases, variable templates, etc. And that also includes calling the compiler builtins, because those are not free. Sure, that's much cheaper than implementing the same logic using pure C++, however builtins still require the compiler to go perform some non-trivial logic, and that has a cost. For example, under the hood, a builtin like `__is_constructible` still has to go and check whether `T(args...)` is well-formed, and that requires doing overload resolution on `T`'s constructor, potentially SFINAEing, etc. I actually don't *know* that that's how it's implemented, but from the outside it has to in order to give the right answer.
So, TLDR: builtins are much cheaper to evaluate than the equivalent pure C++ functionality, but they still are not trivial, and that's what we save by short-circuiting. In comparison, short-circuiting requires us to implement a small constant number of additional templates (like `_Lazy`), whereas calling a single `__is_constructible` builtin could cause any number of types and functions to be instantiated.
================
Comment at: libcxx/include/__expected/unexpected.h:70
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit unexpected(_Error&& __error) //
+ noexcept(is_nothrow_constructible_v<_Err, _Error>)
+ : __unex_(std::forward<_Error>(__error)) {}
----------------
huixie90 wrote:
> philnik wrote:
> > ldionne wrote:
> > > We have discussed adding `noexcept` as an extension several times in the past, and I remember we've done it in a few places. This article explaining the Lakos principle is relevant: https://quuxplusone.github.io/blog/2018/04/25/the-lakos-rule/.
> > >
> > > I think this here is fine. However, I would like to request that we add a new macro to clearly mark that this is an extension. We could call it `_LIBCPP_NOEXCEPT_EXT`. That will simply make it clear that it's an extension so we can grep for those.
> > I think we actually have `noexcept` extensions in quite a few places. What exactly is the benefit of adding a new macro for this? I don't think we ever want to disable it, and I can't really imagine a scenario where you would be interested in `noexcept` extensions specifically. From what I can tell, adding a macro just makes the code harder to read.
> perhaps noticing it is a libc++ extension (rather than what is specified in the standard) is already useful information?
> alternatively we can always add comments.
> I think we actually have `noexcept` extensions in quite a few places.
Yes we do, and I consider it a mistake that we haven't been marking them, since right now we don't have an easy way to know which `noexcept`s are spec-mandated and which ones are "we think it's okay to add it". That's useful information for us to have, and concretely there shouldn't be *that* many places where we add `noexcept` as an extension, so I don't think readability will be such a big concern.
It also calls out behavior that we should be testing in libc++ specific tests.
================
Comment at: libcxx/include/__expected/unexpected.h:104
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(unexpected& __x, unexpected& __y) //
+ noexcept(noexcept(__x.swap(__y)))
----------------
huixie90 wrote:
> curdeius wrote:
> > philnik wrote:
> > > ldionne wrote:
> > > > ldionne wrote:
> > > > > Consider defining this one before `operator==` to make it closer to the member `swap`. That's also how it is in the spec, so bonus points for that.
> > > > I am not sure how much I like this `//` pattern because it's becoming very prevalent. I _really_ like the way you've formatted this though, but I wish clang-format could get that result without so much help. @philnik any opinions on whether we can make `clang-format` do what we want here? Or maybe @curdeius, I think you've worked on `clang-format` a bunch lately?
> > > What exactly are you asking for?
> > > ```lang=c++
> > > _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(unexpected& __x, unexpected& __y) noexcept(noexcept(__x.swap(__y)))
> > > requires is_swappable_v<_Err>
> > > {
> > > __x.swap(__y);
> > > }
> > > ```
> > > seems just fine to me.
> > Do you mean having (non-trivial) noexcept specifier on it's own line?
> > There's no option like this right now, probably because it's not something you have a lot in non-generic code.
> > It should be fairly easy to add an option to break before the noexcept specifier with possible values like the following: never (current behaviour), always (probably not very useful), only before non-trivial specifier i.e. with parentheses.
> > Another option wind be to add a penalty for this, not sure if that suits the needs of libc++ though.
> > So yeah, it would be necessary to clarify the desired formatting.
> Thanks for the comments @philnik @curdeius
> I think this particular one is not bad. But if you look at few lines above
>
> ```
> template <class _Up, class... _Args>
> requires is_constructible_v<_Err, initializer_list<_Up>&, _Args...>
> _LIBCPP_HIDE_FROM_ABI constexpr explicit unexpected(in_place_t, initializer_list<_Up> __il, _Args&&... __args) //
> noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>)
> : __unex_(__il, std::forward<_Args>(__args)...) {}
> ```
>
> If I remove `//`, clang-format would do
> ```
> template <class _Up, class... _Args>
> requires is_constructible_v<_Err, initializer_list<_Up>&, _Args...>
> _LIBCPP_HIDE_FROM_ABI constexpr explicit unexpected(
> in_place_t,
> initializer_list<_Up> __il,
> _Args&&... __args) noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>)
> : __unex_(__il, std::forward<_Args>(__args)...) {}
> ```
>
> It would separates the arguments into separate lines and put `noexcept` on the same line of the last argument, which looks a bit weird
Yup, this is what I'm talking about ^
Generally speaking, it looks like clang-format is quite happy to break lines inside parameter lists instead of breaking before the `noexcept`, which leads to poor indentation.
================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/assert.arrow.pass.cpp:11
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0|12.0}}
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
----------------
I don't think those are required. Can you try removing them and seeing if the CI fails?
If I'm not mistaken, that line would have been added to another test that you copy-pasted from because that other class had some back-deployment requirements. `expected` does not, because we decided not to put the exception class in the dylib.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124516/new/
https://reviews.llvm.org/D124516
More information about the libcxx-commits
mailing list