[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12

Tue Nov 29 21:13:20 PST 2022

On Wed, Nov 30, 2022 at 3:23 AM Louis Dionne via Phabricator <
reviews at reviews.llvm.org> wrote:

> ldionne requested changes to this revision.
> ldionne added a subscriber: jfb.
> ldionne added a comment.
> This revision now requires changes to proceed.
>
> I'm still going to request changes because of the `no_unique_address`
> thing, but this basically LGTM.
>
> Thanks a lot for the great patch and the great collaboration on this
> review. I'm extremely happy with the patch and in particular with the fact
> that we carefully considered the testing coverage during our review of the
> class itself (e.g. order of operations in assignment, etc.).
>
>
>
> ================
> Comment at: libcxx/include/__expected/expected.h:54-56
> +#ifdef _LIBCPP_NO_EXCEPTIONS
> +#  include <cstdlib> // for std::abort
> +#endif
> ----------------
> jwakely wrote:
> > ldionne wrote:
> > > 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.
> > Another option would be to use `__builtin_abort()` which requires no
> header.
> That's interesting, I never thought of doing that. For this review, I
> think we should use `std::abort` to be consistent with what we do
> everywhere else. And then we should review all places in the library where
> we use `std::abort` (which is all the potentially-exception-throwing
> functions) and see whether we can/should use `__builtin_abort` instead.
>
>
> ================
> Comment at: libcxx/include/__expected/expected.h:619-620
> +  union {
> +    _Tp __val_;
> +    _Err __unex_;
> +  };
> ----------------
> huixie90 wrote:
> > ldionne wrote:
> > > 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?
> > Here is my finding.
> >
> > For anonymous union (which is the case in the `expected`, I can't find a
> way to make `[[no_unique_address]]` to have any effect
> > https://godbolt.org/z/zebh4E94d
> >
> > For a named union, if we apply `[[no_unique_address]]` on the `union`
> itself plus all the members of the `union`,  we can save some space.
> > https://godbolt.org/z/rGoa7c47d
> >
> > So I think we don't need to do anything here
> I would suggest using a named union instead so that we can apply
> `[[no_unique_address]]` to the member.
>
> For unnamed unions, it looks like the compiler is applying the attribute
> to the `union` type declaration (not the member declaration). Since
> `[[no_unique_address]]` applies only to member declarations, the attribute
> ends up being ignored when used on an unnamed union.
>
> Also, once that's implemented, let's add a libcxx-specific test that this
> optimization kicks in as intended.
>
>
> ================
> 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))) {
> ----------------
> huixie90 wrote:
> > ldionne wrote:
> > > 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.
> > `noexcept`  does not seem to do SFINAE here. I need to constrain it with
> `requires`
> > https://godbolt.org/z/E15jK4Ter
> I stand corrected. I was *certain* `noexcept` was an immediate context
> that you could SFINAE on. TIL.
>
> I think what you have now with `requires requires` is right. It's OK if it
> fails with clang-15, let's just add the appropriate `XFAIL`s and we'll
> remove them when we drop support for 15.
>
>
> ================
> Comment at:
> libcxx/test/libcxx/utilities/expected/expected.expected/noexcept.extension.compile.pass.cpp:18
> +
> +// constexpr expected();
> +static_assert(std::is_nothrow_default_constructible_v<std::expected<int,
> int>>);
> ----------------
>
>
>
> ================
> Comment at:
> libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp:49
> +    //expected-error-re@*:* {{{{(static_assert|static assertion)}}
> failed {{.*}}argument has to be convertible to value_type}}
> +    //expected-error-re@*:* {{{{no matching conversion for static_cast
> from 'int' to 'NotConvertibleFromInt'}}}}
> +  }
> ----------------
> To get rid of these spurious errors, I would look into this:
>
> ```
> // ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
> ```
>
> See for example
> `libcxx/test/libcxx/atomics/atomics.syn/incompatible_with_stdatomic.verify.cpp`.
>
>
> ================
> Comment at:
> libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp:65
> +    std::expected<NotConvertibleFromInt, int> f1{std::in_place};
> +    std::move(f1).value_or(5);
> +    //expected-error-re@*:* {{{{(static_assert|static assertion)}}
> failed {{.*}}argument has to be convertible to value_type}}
> ----------------
> EricWF wrote:
> > How do we know any of these diagnostics are triggered by the code above
> them? You need to add a check that at least some diagnostic is emitted from
> this line.
> @huixie90  I would suggest moving this to the same line that you're
> expecting the error (i.e. current line 65) and dropping `*:*, which tells
> clang that the error can be on any line of any file (`file:line`). Like
> this:
>
> ```
> std::move(f1).value_or(5); // expected-error-re {{{{(static_assert|static
> assertion)}} failed {{.*}}argument has to be convertible to value_type}}
> ```
>
> That will make for a long line, but it does exactly what I think you want.
>
>
> ================
> Comment at:
> libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp:283
> +  }
> +#endif
> +}
> ----------------
>
>
>
> ================
> Comment at:
> libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp:51
> +
> +constexpr bool test() {
> +  // has_value
> ----------------
> Do we not need exceptions-related tests here to make sure we perform
> operations in the right order?
>
> Please go through all the operations of both the void and the non-void
> specializations. This is a very high bar, but I think it's worth doing it.
>
>
> ================
> Comment at:
> libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp:115-125
> +  // this is a confusing example, but the behaviour
> +  // is exactly what is specified in the spec
> +  {
> +    struct BaseError {};
> +    struct DerivedError : BaseError {};
> +
> +    std::expected<bool, BaseError> e1(false);
> ----------------
> huixie90 wrote:
> > @jwakely Hi Jon, thank you very much for commenting on this patch and
> they are really helpful. While you are here, what do you think about this
> test case? If I switch `bool` to `int`,
> > ```
> > std::expected<int, BaseError> e1(5);
> > std::expected<int, DerivedError> e2(e1);
> > ```
> >
> > the other ctor overload
> >
> > ```
> > expected(const expected<U, G>&)
> > ```
> > will be selected and the `int` value `5` will be copied.
> >
> > But for `bool`, as `T` (`bool`) is constructible from the `expected`,
> the other overload will be disabled and this overload will be selected
> > ```
> > expected(U&&);
> > ```
> > so instead of copying the `bool` (`false`), it uses `operator bool`,
> which is `true` this case.
> >
> > Do you think this behaviour is confusing and not very consistent
> This seems surprising to me. It seems like this might not have been the
> design intent. @jfb perhaps you can comment on this?
>
> Is this worth a LWG issue?
>

Yes for an LWG issue. At a minimum it's surprising.

>
> ================
> Comment at:
> libcxx/test/std/utilities/expected/expected.unexpected/class_mandates/const.compile.fail.cpp:1
>
> +//===----------------------------------------------------------------------===//
> +//
> ----------------
> Please make this into a `verify.cpp`. We're trying to move away from
> `compile.fail.cpp` tests because it's too easy to write a test that fails
> for an unrelated reason.
>
> Applies to all.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D124516/new/
>
> https://reviews.llvm.org/D124516
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20221130/4fd36e11/attachment.html>