This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement `std::expected` P0323R12
ClosedPublic

Authored by huixie90 on Apr 27 2022, 3:33 AM.

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
huixie90 added inline comments.Nov 1 2022, 1:21 AM
libcxx/include/__expected/expected.h
311

clang 14 does not support multiple destructors

613–617

I tried several examples and I could not find an example that the order of these two member affects the size of the class.

jwakely added inline comments.Nov 1 2022, 3:26 AM
libcxx/include/__expected/expected.h
109

@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.

The pattern comes from optional's converting ctors, see http://wg21.link/optional.ctor#27 and was added by https://wg21.link/lwg2756 which was originally done for experimental::optional in https://wg21.link/lwg2451 which links to the "Odd" example at https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4064.html#perfect_init as rationale.

IIRC it's there to prevent the ctor being a candidate for such odd types where direct-init would fail but copy-init would succeed.

311

clang 14 does not support multiple destructors

And __cpp_concepts >= 202002L is the correct check for that, as specified by https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2493r0.html

libcxx/include/__expected/unexpected.h
71

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.

huixie90 added inline comments.Nov 1 2022, 5:25 AM
libcxx/include/__expected/expected.h
631–634

@jwakely Hi Jon, I have noticed that in libstdc++, there is a place holder type as the first type in the union
https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/expected#L893

I am trying to understand the purpose of that type. At the first glance, I thought it is for the case where T is not default constructible and we need that type in case we cannot initialise T directly in the expected's constructor's initializer lists.
It turns out that I was wrong and it is possible to have the union uninitialised even if T is not default constructible.
https://godbolt.org/z/9xePvrE4x

So I believe libstdc++ has that dummy type for other reasons. Could you please shed some lights on this?

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp
116–126

@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

huixie90 marked an inline comment as not done.Nov 1 2022, 5:26 AM
jwakely added inline comments.Nov 1 2022, 6:47 AM
libcxx/include/__expected/expected.h
631–634

So I believe libstdc++ has that dummy type for other reasons. Could you please shed some lights on this?

I think it was necessary with older versions of GCC, but isn't needed now. It could be removed.

EricWF requested changes to this revision.Nov 1 2022, 4:56 PM
EricWF added a subscriber: EricWF.

Please add a link to the paper in the change description. As well as a meaningful description for the change.
Consider including any information you would want a reader in 5 years to have when they're puzzling over your code.

Also there's no reason to split this up into multiple headers. There are no deps we can cut from other headers by doing so, nor any circular dependency issues we're working around.

Another improvement that can be made is reducing the number of instantiations we perform in a lot of the metaprogramming. _And, _Not, and _Or should only be used when short circuiting is needed. In most cases here it isn't. Every single instantiation performed in expected is carried around by the compiler and in the AST for the entire translation unit. That's costly, lets avoid that cost.

libcxx/include/__expected/bad_expected_access.h
40

Why are we hiding this from the ABI? It's a vtable function, so I don't see how that's even possible?

libcxx/include/__expected/expected.h
70

This needs to be hidden from the ABI. Perhaps always inlined or internal linkage.

76–90

I agree. The short circuiting in the static assert doesn't provide value.

Either it evaluates to the end, or it fails early and the program stops compiling.

So many unnecessary instantiations.

105

Just de morgan this entire thing?

120

Doesn't this have linkage?

223

Shouldn't other.__has_val_ be

594

Is there a reason to not just write the member access into the union? It would save function calls and instantiations, no?

601

Is there a reason to write this using the ternary operator? My spidey senses think that it's easier to read if you don't need to guess at the common type of the operator, and validate that it isn't something weird.

613–617

So the difference is where the padding goes. If the bool comes first the padding goes between the between the bool and the values, potentially causing the type to span over two cache lines or multiple registers.

Whereas if the bool is second, the padding will come after the bool at the end of the type. Which is significantly preferable.

755

I don't think we can do this as it potentially changes the ABI of the type, no?

And that'll be ABI breaking between the two preprocessor branches.

libcxx/include/__expected/unexpected.h
52

Don't write these metaprograms like this.

It's significantly less expensive in terms of compile time, and compiler memory to just turn the results into bools and operate on them that way.

These traits can't blow up when evaluated, so there's no reason to make them lazy.

In fact, we can do away with the entire class like `template <class T> using __valid_unexpected = bool_constant<is_object_v<_Tp> && !is_array<_Tp>...>;`

libcxx/include/__type_traits/is_member_pointer.h
15

What's up with this change?

libcxx/include/__type_traits/is_scalar.h
16

Are these just merge conflicts / weird rebases? Because I don't understand this at all.

libcxx/include/__type_traits/is_void.h
15

What?

libcxx/include/expected
12

Why not define the things that are in this header, in this header?

Is there any reason to split them up? Like dependencies or the like?

Because every additional header comes at a cost, and so it needs to provide value to pay that cost.

libcxx/test/std/utilities/expected/expected.void/ctor/ctor.unexpect_init_list.pass.cpp
56

Is this requires clause necessary for the correctness of the test?

libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
34

What does this test?

59

We're missing test coverage on the const&& and & overloads of value().

This revision now requires changes to proceed.Nov 1 2022, 4:56 PM
EricWF added inline comments.Nov 1 2022, 4:59 PM
libcxx/include/__expected/expected.h
928

Oh god, we can compare error types for equality that don't produce a boolean output. That's terrifying. I assume that's what the standard wants, but that's terrifying.

934

You can avoid the function calls by touching the members directly. That'll save in non-optimized build size.

938

Reverse the order of these members please.

EricWF added inline comments.Nov 1 2022, 5:06 PM
libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp
58 ↗(On Diff #472233)

Is the deleted constructor not on one of the user-provided types?

65 ↗(On Diff #472233)

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.

EricWF added inline comments.Nov 1 2022, 5:36 PM
libcxx/include/__expected/expected.h
223

Ignore me. I had a dumb thought, but forgot to delete the comment.

huixie90 marked 3 inline comments as done.Nov 2 2022, 5:32 AM
huixie90 added inline comments.
libcxx/include/__expected/expected.h
76–90

I agree. The short circuiting in the static assert doesn't provide value.

Either it evaluates to the end, or it fails early and the program stops compiling.

So many unnecessary instantiations.

76–90

I agree. The short circuiting in the static assert doesn't provide value.

Either it evaluates to the end, or it fails early and the program stops compiling.

So many unnecessary instantiations.

76–90

I agree. The short circuiting in the static assert doesn't provide value.

Either it evaluates to the end, or it fails early and the program stops compiling.

So many unnecessary instantiations.

This particular thread is not about static_assert. Sorry for the confusion as the code has been rearranged since the beginning of the thread. This thread was about the __can_convert, which has lots of boolean conditions with && and ||. It is not used in static_assert, but used in lots of constructors' constraints.
I do believe shorting circuiting is worth it (see Louis's comment above)

105

could you please clarify? The code has been rearranged since Louis's comment above and it is not clear what line this comment was about

594

The standard spec uses function calls. I did try to change it to direct access but I missed some while copy-pasting from the spec

601

This is how it is specified in the spec. I guess if I wrote in a different way, then it requires the reader to figure out if the way I wrote is equivalent to what is in the spec (the ternary operator)

libcxx/include/__type_traits/is_member_pointer.h
15

clearly, the if condition is not correct. it should be

#if !__has_builtin(__is_member_pointer)

The expected uses this header and I get failures when doing the modular build. This file uses __libcpp_is_member_pointer on line38, but without including the header <__type_traits/is_member_function_pointer.h> that defines it.

libcxx/include/__type_traits/is_scalar.h
16

I believe it was the modular build error message suggested to include this header. I think the actual missing include is

#include <__type_traits/is_null_pointer.h>
libcxx/include/__type_traits/is_void.h
15

yes . this should be

#if !__has_builtin(__is_void)

But the #include <__type_traits/is_same.h> is indeed needed as it is used on line 38.

I am wondering how come I am the only one had issues with these missing includes

libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp
58 ↗(On Diff #472233)

I think the error message contains some implementation detail code that looks like

call to deleted constructor of <some templates> {aka <user defined type>}

I think I can add some regex to include the user defined type

libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
34

Yes in this case std::expected<void, T>::value is just returning void without doing anything useful. But we still need to test it to make sure

  1. it is implemented (not just declared)
  2. it does not throw (as supposed to the error state where it does throw)
huixie90 marked 10 inline comments as done.Nov 2 2022, 1:33 PM
huixie90 added inline comments.
libcxx/test/libcxx/utilities/expected/expected.expected/assert.arrow.pass.cpp
11 ↗(On Diff #470382)

It turns out that after removing this, the test does fail on the Apple systems

https://reviews.llvm.org/harbormaster/unit/view/5402162/

huixie90 updated this revision to Diff 472742.Nov 2 2022, 1:33 PM
  • address feedback

Another improvement that can be made is reducing the number of instantiations we perform in a lot of the metaprogramming. _And, _Not, and _Or should only be used when short circuiting is needed. In most cases here it isn't. Every single instantiation performed in expected is carried around by the compiler and in the AST for the entire translation unit. That's costly, lets avoid that cost.

I've removed short circuiting for the static_assert cases. but most meta programming in this patch can benefit from short-circuiting as they are used in the constructors' constraints. (the most complicated one in this patch is the __can_convert @ldionne had some comments explaining why they are needed. (in the early revisions of this patch, there was no short-circuiting)

That being said, I wrote sine simple tests that benchmark the compilation times.

The first one is the case where short circuiting happens

// The 3rd requirement of `__can_convert`
// _Not<is_constructible<_Tp, expected<_Up, _Gp>&>>
// fails and short circuits
template <std::size_t N>
struct Foo {
  Foo() {}

  template <std::size_t M>
  Foo(Foo<M>) {}

  template <std::size_t M, class E>
  Foo(std::expected<Foo<M>, E>) {}
};

template <std::size_t N>
void callCtor() {
  std::expected<Foo<N>, int> e1;
  [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
}

template <std::size_t... Ns>
void generateTest(std::index_sequence<Ns...>) {
  (callCtor<Ns>(), ...);
}

int main() { generateTest(std::make_index_sequence<1000>{}); }
  • With the _And/_Or version, it took *8.9s* to compile.
  • With the _BoolConstant<is_xxx_v && ...> version, it took *10.0s* to compile

So we see some benefits of short circuiting (out weighs the instantiation cost)

Then I have another test case where all boolean requirements pass so no short circuiting happening

// All requirements of `__can_convert` pass
// short circuiting has no benefit
template <std::size_t N>
struct Foo {
  Foo() {}

  template <std::size_t M>
  Foo(Foo<M>) {}
};

template <std::size_t N>
void callCtor() {
  std::expected<Foo<N>, int> e1;
  [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
}

template <std::size_t... Ns>
void generateTest(std::index_sequence<Ns...>) {
  (callCtor<Ns>(), ...);
}

int main() { generateTest(std::make_index_sequence<1000>{}); }
  • With the _And/_Or version, it took *8.5s* to compile.
  • With the _BoolConstant<is_xxx_v && ...> version, it took *7.3s* to compile

So we know that short circuiting doesn't help because all booleans are true. And the instantiation costs make it slower.

The first case we gain 1.1s and the second case we lost 1.2s. So I am on the boarder line of whether or not to do short circuiting on these constructor constraints. I think in the real case, it is likely that those booleans are all true because if the user calls these converting constructors, it is likely to succeed instead of falling back to the constructor that takes U&&. That means in real user code, the short circuiting might never happen.

huixie90 edited the summary of this revision. (Show Details)Nov 2 2022, 2:37 PM
EricWF added a comment.Nov 6 2022, 3:11 PM

Another improvement that can be made is reducing the number of instantiations we perform in a lot of the metaprogramming. _And, _Not, and _Or should only be used when short circuiting is needed. In most cases here it isn't. Every single instantiation performed in expected is carried around by the compiler and in the AST for the entire translation unit. That's costly, lets avoid that cost.

I've removed short circuiting for the static_assert cases. but most meta programming in this patch can benefit from short-circuiting as they are used in the constructors' constraints. (the most complicated one in this patch is the __can_convert @ldionne had some comments explaining why they are needed. (in the early revisions of this patch, there was no short-circuiting)

That being said, I wrote sine simple tests that benchmark the compilation times.

The first one is the case where short circuiting happens

// The 3rd requirement of `__can_convert`
// _Not<is_constructible<_Tp, expected<_Up, _Gp>&>>
// fails and short circuits
template <std::size_t N>
struct Foo {
  Foo() {}

  template <std::size_t M>
  Foo(Foo<M>) {}

  template <std::size_t M, class E>
  Foo(std::expected<Foo<M>, E>) {}
};

template <std::size_t N>
void callCtor() {
  std::expected<Foo<N>, int> e1;
  [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
}

template <std::size_t... Ns>
void generateTest(std::index_sequence<Ns...>) {
  (callCtor<Ns>(), ...);
}

int main() { generateTest(std::make_index_sequence<1000>{}); }
  • With the _And/_Or version, it took *8.9s* to compile.
  • With the _BoolConstant<is_xxx_v && ...> version, it took *10.0s* to compile

So we see some benefits of short circuiting (out weighs the instantiation cost)

Then I have another test case where all boolean requirements pass so no short circuiting happening

// All requirements of `__can_convert` pass
// short circuiting has no benefit
template <std::size_t N>
struct Foo {
  Foo() {}

  template <std::size_t M>
  Foo(Foo<M>) {}
};

template <std::size_t N>
void callCtor() {
  std::expected<Foo<N>, int> e1;
  [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
}

template <std::size_t... Ns>
void generateTest(std::index_sequence<Ns...>) {
  (callCtor<Ns>(), ...);
}

int main() { generateTest(std::make_index_sequence<1000>{}); }
  • With the _And/_Or version, it took *8.5s* to compile.
  • With the _BoolConstant<is_xxx_v && ...> version, it took *7.3s* to compile

So we know that short circuiting doesn't help because all booleans are true. And the instantiation costs make it slower.

The first case we gain 1.1s and the second case we lost 1.2s. So I am on the boarder line of whether or not to do short circuiting on these constructor constraints. I think in the real case, it is likely that those booleans are all true because if the user calls these converting constructors, it is likely to succeed instead of falling back to the constructor that takes U&&. That means in real user code, the short circuiting might never happen.

Thank you for looking into this so thoroughly. I should have better specified that my concerns were with meta-programming in static_asserts and other places where all of the conditions were likely to get evaluated. For constraints, I think the opposite is true, and lazy metaprogramming is the way to go.

libcxx/include/__expected/expected.h
76–90

Sorry, it looked like it was attached to the static assert on my end.

For constraints I'm all for lazy evaluation (and think it may even be needed).
But for anything that's going to get evaluated fully anyway, then just do it eagerly.

ldionne added inline comments.Nov 15 2022, 11:34 AM
libcxx/include/__expected/bad_expected_access.h
40

We played around with it a bit: https://godbolt.org/z/fczzo8q5d

It seems like it doesn't make any difference. The compiler will still emit a virtual call whether it has the attribute always-inline (GCC) or abi-tag (Clang). However, since this method is in the vtable, it arguably can't change in any way it wants, so I think that using _LIBCPP_HIDE_FROM_ABI is misleading in that case. I think we should remove it from what() and from ~bad_expected_access().

I *think* we want to make it visibility=hidden, however, because we don't want people's shared libraries to start exporting this symbol.

Also, as a side note, the actual mangled name of this function doesn't matter too much, because it's almost always accessed as an index into the vtable of bad_expected_access<void>.

libcxx/include/__expected/expected.h
311

We don't often use feature-test macros to check for the availability of stuff. Instead, we check for compiler versions, which allows us to get rid of workarounds as the codebase evolves and we drop support for older compilers.

Yes, I know that's partly why feature-test macros were designed, but I think they provide more value to users than to us given our compiler support policy.

755

I think this is a valid concern, and I seem to have missed that in my review. Thanks a lot for bringing this up.

IMO this means that we can only support compilers where __cpp_concepts >= 202002. If that means that std::expected doesn't work on Clang <= 15, I think that's fine. It'll be a bit annoying to add XFAIL annotations to the expected tests, but concretely it won't change anything for users because we release a matching Clang and libc++. And those XFAIL annotations will be cleaned up as our support window moves.

libcxx/include/expected
12

Speaking with @huixie90 , he split them off originally to aid readability and because they are logically separated classes. That makes sense to me, and IMO the code is easier to understand in its current layout than if we put everything in the same 1300+ lines header.

We often split up much more granularly than this, so I'm not sure the cost of this split here is really meaningful.

27

The synopsis usually comes before all the sub-headers (see for example <optional>).

libcxx/test/libcxx/utilities/expected/expected.expected/assert.arrow.pass.cpp
11 ↗(On Diff #470382)

This looks like a bug (in the test suite) to me. We should be defining _LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED whenever we override __libcpp_verbose_abort in the test suite, but we don't.

I think many existing tests are incorrectly using XFAIL.

You can either fix this in a prior patch, or re-add the XFAIL and we can fix all of them in a follow-up patch. I think this is the most reasonable approach to avoid blocking this review on a pre-existing issue.

huixie90 updated this revision to Diff 476204.Nov 17 2022, 12:09 PM
huixie90 marked 20 inline comments as done.
  • address comment
huixie90 updated this revision to Diff 476759.Nov 20 2022, 10:27 AM

temporily reverted modules_includes.sh.cpp and headers_declara_verbose_abort.sh.cpp

ldionne added inline comments.Nov 23 2022, 10:35 AM
libcxx/include/__expected/expected.h
288
434

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.

446
630–631

This feels simpler to me. Also consider rewording the one for non-void T above.

636–643
692–694

Here and to the other overloads below too.

744

This needs to go too!

769–770

Please ensure you have a test for this under libcxx/test/std since this is not an extension (extension noexcepts should be tested too, though). I guess what I mean is "please test all the noexcepts, whether they are extensions or not".

800–801

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.

818–819

Also here, a test for the order of statements.

830–831

Same, order of statements.

846

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.

850

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 expecteds
  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.
864

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.

libcxx/include/__expected/unexpected.h
71

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.

libcxx/test/libcxx/utilities/expected/expected.expected/assert.arrow.pass.cpp
1 ↗(On Diff #477004)

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
ldionne added inline comments.Nov 23 2022, 10:35 AM
libcxx/include/__expected/expected.h
55–57

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.

620–621

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?

870

Please make sure you check that this is explicit, i.e. an implicit conversion to bool should fail.

884
927–930

Let's match the order of members in the non-void case.

929

Same comment for no_unique_address.

libcxx/include/__expected/unexpected.h
71

Can you please add a test for this default template argument? See Jonathan's comment above explaining where this comes from.

120–121

Please ensure you have tests for this deduction guide.

libcxx/include/__type_traits/is_member_pointer.h
15–17

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.

libcxx/include/__type_traits/is_nothrow_constructible.h
15–18

Same

libcxx/include/__type_traits/is_void.h
15–18

Same

libcxx/include/expected
54
jwakely added inline comments.Nov 23 2022, 11:17 AM
libcxx/include/__expected/expected.h
55–57

Another option would be to use __builtin_abort() which requires no header.

huixie90 updated this revision to Diff 477836.Nov 24 2022, 12:56 PM
huixie90 marked 19 inline comments as done.

more CI

huixie90 added inline comments.Nov 24 2022, 2:57 PM
libcxx/include/__expected/expected.h
620–621

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

huixie90 updated this revision to Diff 477981.Nov 25 2022, 8:48 AM
huixie90 marked 11 inline comments as done.

address comments

huixie90 added inline comments.Nov 25 2022, 9:41 AM
libcxx/include/__expected/expected.h
864

noexcept does not seem to do SFINAE here. I need to constrain it with requires
https://godbolt.org/z/E15jK4Ter

huixie90 updated this revision to Diff 478095.Nov 27 2022, 6:37 AM

next try CI

ldionne requested changes to this revision.Nov 29 2022, 10:23 AM
ldionne added a subscriber: jfb.

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.).

libcxx/include/__expected/expected.h
55–57

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.

620–621

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.

864

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 XFAILs and we'll remove them when we drop support for 15.

libcxx/test/libcxx/utilities/expected/expected.expected/noexcept.extension.compile.pass.cpp
18 ↗(On Diff #478095)
libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp
49 ↗(On Diff #478095)

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.

65 ↗(On Diff #472233)

@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.

libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp
284
libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
52

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.

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp
116–126

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?

libcxx/test/std/utilities/expected/expected.unexpected/class_mandates/const.compile.fail.cpp
2

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.

This revision now requires changes to proceed.Nov 29 2022, 10:23 AM
huixie90 updated this revision to Diff 478682.Nov 29 2022, 12:35 PM

named union and no_unique_address

jwakely added inline comments.Nov 30 2022, 1:23 AM
libcxx/include/__expected/expected.h
864

I stand corrected. I was *certain* noexcept was an immediate context that you could SFINAE on. TIL.

No, [temp.deduct.general] p7 is very explicit about that. noexcept doesn't participate in overload resolution, and doesn't even need to be instantiated unless checked ([temp.inst] p14).

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp
116–126

The rules are supposed to allow initialization of a result type that can be constructed from expected, but because expected<T,U> is always contextually convertible to bool, that kicks in here. This reminds me of https://wg21.link/p0608 where we also needed to exclude bool from being greedy. It shouldn't be a problem for anything other than bool.

Please do file a library issue, thanks.

huixie90 updated this revision to Diff 479077.Nov 30 2022, 2:46 PM
huixie90 marked 9 inline comments as done.

CI

libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp
65 ↗(On Diff #472233)

For some reason, this does not work and the compiler thinks the error was emitted from expected.h instead of this particular line in the test

error: 'error' diagnostics expected but not seen: 
  File /Users/huixie/repos/libc++/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp Line 38: {{(static_assert|static assertion)}} failed {{.*}}value_type has to be copy constructible
error: 'error' diagnostics seen but not expected: 
  File /Users/huixie/repos/libc++/llvm-project/build/include/c++/v1/__expected/expected.h Line 595: static assertion failed due to requirement 'is_copy_constructible_v<NonCopyable>': value_type has to be copy constructible
huixie90 updated this revision to Diff 479847.Dec 3 2022, 9:44 AM
huixie90 marked 5 inline comments as done.

address remaining comments

libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
52

turns out that the emplace function requires is_nothrow_constructible_v so we don't need a runtime tests. (there are compile time tests above to test that if it can throw , there will be no emplace function)

ldionne accepted this revision.Dec 13 2022, 9:28 AM

@huixie90 Thank you so much for this great patch. It's not an easy one but I'm extremely happy with the state it has reached now, and I think we've addressed everyone's comments. I'm excited to ship this in LLVM 16!

I still left a few comments for minor nits, but feel free to ship this once they are addressed.

Thank you, and thanks to other folks who chimed in on the review!

libcxx/include/__expected/expected.h
643–645
864

Thanks for the ref and for schooling me :-).

libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp
65 ↗(On Diff #472233)

I actually think that's expected, in retrospect. This is what happens for all of our static_assert tests. I think what you've got right now is fine, but if you wanted, you could also pin this down a bit more by:

  1. Using //expected-error-re@expected.h:* to make sure that the static_assert doesn't come from e.g. <vector> (not sure how useful that is, but you could)
  2. Adding //expected-note {{in instantiation}} (you might have to figure out the right content inside {{}}) on the line of the std::move(...).value_or(...) call. That would ensure that Clang did indeed generate *some* diagnostic starting on that line. I think that would be way sufficient.

FWIW, I'm happy with the test as-is too.

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp
116–126

Thanks for confirming. @huixie90 Can you please add a link to the LWG issue in the comment above?

huixie90 updated this revision to Diff 482792.Dec 14 2022, 3:33 AM
huixie90 marked 11 inline comments as done.

rebase

This revision was not accepted when it landed; it landed in state Needs Review.Dec 14 2022, 7:45 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.