clang 14 does not support multiple destructors

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

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

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.

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

@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

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.

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.

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

Reverse the order of these members please.

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

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.

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

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.

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.

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)

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

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

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)

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.

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>

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

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

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)

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

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

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.

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

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.

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.

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.

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

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.

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.

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

Here and to the other overloads below too.

This needs to go too!

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

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.

Also here, a test for the order of statements.

Same, order of statements.

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.

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.

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.

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.

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

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.

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?

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

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

Same comment for no_unique_address.

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

Please ensure you have tests for this deduction guide.

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.

Same

Same

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

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

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.

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

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.