[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Nov 23 10:35:59 PST 2022
ldionne added inline comments.
================
Comment at: libcxx/include/__expected/expected.h:54-56
+#ifdef _LIBCPP_NO_EXCEPTIONS
+# include <cstdlib> // for std::abort
+#endif
----------------
IMO the simplicity of always having the same set of transitive includes outweighs the additional `<cstdlib>` include, especially since I think most users will end up including that header anyway. I would just include this header unconditionally.
================
Comment at: libcxx/include/__expected/expected.h:287
+ is_nothrow_move_constructible_v<_T2>,
+ "To provide strong exception guarrentee, T2 has to satisfy `is_nothrow_move_constructible_v` so that it can "
+ "be reverted to the previous state in case an exception is thrown during the assignment.");
----------------
================
Comment at: libcxx/include/__expected/expected.h:433
+private:
+ _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>) {
----------------
I would consider moving this to a local lambda of `swap` too, but it may end up being too nasty since this one is much more complicated than the `void` case. I'll let you judge.
================
Comment at: libcxx/include/__expected/expected.h:445
+ static_assert(is_nothrow_move_constructible_v<_Tp>,
+ "To provide strong exception guarrentee, Tp has to satisfy `is_nothrow_move_constructible_v` so "
+ "that it can be reverted to the previous state in case an exception is thrown during swap.");
----------------
================
Comment at: libcxx/include/__expected/expected.h:619-620
+ union {
+ _Tp __val_;
+ _Err __unex_;
+ };
----------------
Can we use `[[no_unique_address]]` here? Can you investigate how that attribute interacts with unions? Also, does it make sense to put it on the union itself?
================
Comment at: libcxx/include/__expected/expected.h:629-630
+ static_assert(__unexpected::__valid_unexpected<_Err>::value,
+ "[expected.void.general] A program that instantiates the definition of the template expected<T, E> "
+ "with a type for the E parameter that is not a valid template argument for unexpected is ill-formed");
+
----------------
This feels simpler to me. Also consider rewording the one for non-`void` `T` above.
================
Comment at: libcxx/include/__expected/expected.h:635-642
+ template <class _Up, class _Gp, class _Gf>
+ using __can_convert = _And< //
+ is_void<_Up>, //
+ is_constructible<_Err, _Gf>, //
+ _Not<is_constructible<unexpected<_Err>, expected<_Up, _Gp>&>>, //
+ _Not<is_constructible<unexpected<_Err>, expected<_Up, _Gp>>>, //
+ _Not<is_constructible<unexpected<_Err>, const expected<_Up, _Gp>&>>, //
----------------
================
Comment at: libcxx/include/__expected/expected.h:691-693
+ template <class _Up, class _Gp>
+ requires __can_convert<_Up, _Gp, const _Gp&>::value
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _Gp&, _Err>) //
----------------
Here and to the other overloads below too.
================
Comment at: libcxx/include/__expected/expected.h:743
+
+# if __cpp_concepts >= 202002
+ _LIBCPP_HIDE_FROM_ABI constexpr ~expected()
----------------
This needs to go too!
================
Comment at: libcxx/include/__expected/expected.h:768-769
+ _LIBCPP_HIDE_FROM_ABI constexpr expected& operator=(const expected& __rhs) //
+ noexcept((is_nothrow_copy_assignable_v<_Err> && //
+ is_nothrow_copy_constructible_v<_Err>))
+ requires(is_copy_assignable_v<_Err> && //
----------------
Please ensure you have a test for this under `libcxx/test/std` since this is not an extension (extension `noexcept`s should be tested too, though). I guess what I mean is "please test all the noexcepts, whether they are extensions or not".
================
Comment at: libcxx/include/__expected/expected.h:799-800
+ if (!__rhs.__has_val_) {
+ std::construct_at(std::addressof(__unex_), std::move(__rhs.__unex_));
+ __has_val_ = false;
+ }
----------------
I am going to be suuuuper pedantic and request a test that we're doing things in the right order here. Basically, if the construction of `unex` throws, we must not have engaged `*this`, and the spec is explicit about this because they describe this operation as
```
construct_at(addressof(unex), std::move(rhs.unex));
has_val = false;
```
This is a bit nitpicky, but I think it would be nice to have a test that fails if we reorder these two statements. This also applies to the `const&` overload above and to the converting overloads below. If there's a uniform way of testing that requirement, feel free to jam all the test cases for this property into the same file to reduce duplication.
================
Comment at: libcxx/include/__expected/expected.h:817-818
+ if (__has_val_) {
+ std::construct_at(std::addressof(__unex_), __un.error());
+ __has_val_ = false;
+ } else {
----------------
Also here, a test for the order of statements.
================
Comment at: libcxx/include/__expected/expected.h:829-830
+ if (__has_val_) {
+ std::construct_at(std::addressof(__unex_), std::move(__un.error()));
+ __has_val_ = false;
+ } else {
----------------
Same, order of statements.
================
Comment at: libcxx/include/__expected/expected.h:845
+ // [expected.void.swap], swap
+ _LIBCPP_HIDE_FROM_ABI constexpr void swap(expected& __rhs) //
+ noexcept((is_nothrow_move_constructible_v<_Err> && is_nothrow_swappable_v<_Err>))
----------------
I would suggest doing this instead:
```
... decl ... {
auto __swap_impl = [](auto& __with_val, auto& __with_err) {
std::construct_at(std::addressof(__with_val.__unex_), std::move(__with_err.__unex_));
std::destroy_at(std::addressof(__with_err.__unex_));
__with_val.__has_val_ = false;
__with_err.__has_val_ = true;
};
if (__has_val_) {
if (!__rhs.__has_val_) {
__swap_impl(*this, __rhs);
}
}
}
```
And then you can get rid of `__swap_val_unex_impl`. If you don't get rid of it, at least please move it close so we can see the definition.
================
Comment at: libcxx/include/__expected/expected.h:849
+ {
+ if (__has_val_) {
+ if (!__rhs.__has_val_) {
----------------
Can you make sure you have a test where you `swap` and the error throws when being moved out-of, and ensure that:
1. We have not changed the `has_value()` for either of the `expected`s
2. And also that the `expected` that contained an `Err` before still does, and that the `Err` has not been destroyed. An implementation failing this would be silly overcomplicated, but let's still check this.
================
Comment at: libcxx/include/__expected/expected.h:863
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(expected& __x, expected& __y) //
+ noexcept(noexcept(__x.swap(__y))) {
----------------
Please make sure you have tests that check that this is "equivalent to" the member swap, i.e. that the same constraints apply. This should be SFINAE friendly if the member swap can't be used, basically.
================
Comment at: libcxx/include/__expected/expected.h:869
+ // [expected.void.obs], observers
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit operator bool() const noexcept { return __has_val_; }
+
----------------
Please make sure you check that this is explicit, i.e. an implicit conversion to `bool` should fail.
================
Comment at: libcxx/include/__expected/expected.h:883
+
+ constexpr void value() && {
+ if (!__has_val_) {
----------------
================
Comment at: libcxx/include/__expected/expected.h:926-929
+ bool __has_val_;
+ union {
+ _Err __unex_;
+ };
----------------
Let's match the order of members in the non-`void` case.
================
Comment at: libcxx/include/__expected/expected.h:928
+ union {
+ _Err __unex_;
+ };
----------------
Same comment for `no_unique_address`.
================
Comment at: libcxx/include/__expected/unexpected.h:70
+
+ template <class _Error = _Err>
+ requires(!is_same_v<remove_cvref_t<_Error>, unexpected> && //
----------------
Can you please add a test for this default template argument? See Jonathan's comment above explaining where this comes from.
================
Comment at: libcxx/include/__expected/unexpected.h:119-120
+
+template <class _Err>
+unexpected(_Err) -> unexpected<_Err>;
+
----------------
Please ensure you have tests for this deduction guide.
================
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)) {}
----------------
jwakely wrote:
> ldionne wrote:
> > 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.
> > 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.
>
> IIRC MSVC just adds a comment saying `// strengthened` where they've added a non-required `noexcept`. That seems much lower overhead than a macro. Why use a macro if you never want it to be defined to anything except `noexcept`?
>
> Libstdc++ doesn't bother, we just add it where it makes sense. It's just good QoI.
>
> And if the standard says "*Throws*: Nothing." then that means it's **required** to never throw, so adding it is the right thing to do.
>
> N.B. LEWG have decided to stop following the Lakos Rule, although that direction hasn't been approved at plenary yet. It's the right direction IMHO.
>
> IIRC MSVC just adds a comment saying `// strengthened` where they've added a non-required `noexcept`.
I'd be fine with that too. I just want a visual cue that something is an extension.
================
Comment at: libcxx/include/__type_traits/is_member_pointer.h:15-17
+#if !__has_builtin(__is_member_pointer)
+#include <__type_traits/is_member_function_pointer.h>
+#endif
----------------
Likewise here, I wouldn't bother with the conditional include. We'll eventually be able to remove it anyway once all supported compilers implement the builtin.
================
Comment at: libcxx/include/__type_traits/is_nothrow_constructible.h:15-18
+#if !__has_builtin(__is_nothrow_constructible)
+#include <__type_traits/is_reference.h>
+#include <__utility/declval.h>
+#endif
----------------
Same
================
Comment at: libcxx/include/__type_traits/is_void.h:15-18
+#if !__has_builtin(__is_void)
+#include <__type_traits/is_same.h>
+#include <__type_traits/remove_cv.h>
+#endif
----------------
Same
================
Comment at: libcxx/include/expected:53
+
+#endif // _LIBCPP_EXEPECTED
+
----------------
================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/assert.arrow.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Not attached to a line: I think this should fix your CI issues:
```
// Older Clangs do not support the C++20 feature to constrain destructors
// XFAIL: clang-14, apple-clang-14
```
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