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

Tue Nov 29 10:23:08 PST 2022

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?

================
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