This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement P2505R5(Monadic operations for std::expected).
ClosedPublic

Authored by yronglin on Jan 3 2023, 10:40 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Can someone tell me how to fix this clang_query.sh.cpp failure? Should I need to update any other files, I'm very confused, many thanks!

This failure wasn't your fault. I've committed a fix, so it should be fixed with a rebase.

Can someone tell me how to fix this clang_query.sh.cpp failure? Should I need to update any other files, I'm very confused, many thanks!

This failure wasn't your fault. I've committed a fix, so it should be fixed with a rebase.

Thank you for your reply @philnik !

yronglin updated this revision to Diff 487742.Jan 10 2023, 4:03 AM

try fix test

yronglin updated this revision to Diff 487757.Jan 10 2023, 5:07 AM

try fix test

Thanks for working on this! I didn't do a real review, mainly glanced over it and I have a few remarks.

libcxx/docs/Status/Cxx2bPapers.csv
103
103

Can you update libcxx/docs/ReleaseNotes.rst too?

libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp
164

We don't require clang-format, so I have a slight preference to remove this.

Thanks for working on this! I didn't do a real review, mainly glanced over it and I have a few remarks.

Thanks for your comments @Mordante ,I'll try to update this patch

philnik added inline comments.Jan 10 2023, 8:49 AM
libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp
164

I'd keep it, since it allows you to clang-format the file if you change anything. I actually do this for the files which look formatted regularly.

yronglin updated this revision to Diff 487825.Jan 10 2023, 8:56 AM

Update ReleaseNotes
Remove clang-format comments in test

yronglin marked 2 inline comments as done.Jan 10 2023, 9:01 AM
yronglin added inline comments.
libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp
164

Yes, this was added in the past to facilitate formatting files, otherwise the format here will be disrupted by clang-format, I can keep these if you guys agree

yronglin updated this revision to Diff 487830.Jan 10 2023, 9:04 AM
yronglin marked an inline comment as done.

Keep clang-format in test

yronglin added inline comments.Jan 10 2023, 9:12 AM
libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp
164

If we remove // clang-format off ......// clang-format on.... , the code here will be formatted in a very strange format(I've used vscode Format Document)

yronglin updated this revision to Diff 488054.Jan 10 2023, 6:21 PM
yronglin marked an inline comment as done.

Update libcxx/docs/Status/Cxx2bPapers.csv

yronglin marked an inline comment as done.
philnik added inline comments.Jan 15 2023, 9:34 AM
libcxx/include/__expected/expected.h
67

I would remove the namespace then. It doesn't really improve the readability in any way, since every single member has expected already in it's name. We also don't have these kinds of detail namespaces normally.

libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp
10 ↗(On Diff #488054)

This seems weird. Why should we ignore errors?

libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp
80
155

Same for the others.

170–173
libcxx/test/std/utilities/expected/expected.void/observers/error_or.pass.cpp
94

Why is this only static_asserted?

yronglin marked 4 inline comments as done.Jan 16 2023, 5:53 AM

Many thanks for your comments @philnik !

libcxx/include/__expected/expected.h
67

If we decided to remove expected namespace, I think unexpected namespace in unexpected.h also should be removed, What do you think?

libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp
10 ↗(On Diff #488054)

This used to ignore errors like type '_Up' (aka 'int') cannot be used prior to '::' because it has no members, no matching constructor for initialization of '_Up' (aka 'std::expected<int, NotSameAsInt>'), and so on, I think we don't need to check errors like that, what do you think?

libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp
2–7

+1

18

+1

libcxx/test/std/utilities/expected/expected.void/observers/error_or.pass.cpp
94

Could you please clarify?

yronglin marked 5 inline comments as done.Jan 16 2023, 5:54 AM
yronglin added inline comments.
libcxx/include/__expected/expected.h
67

If we decided to remove __expected namespace, I think __unexpected namespace in unexpected.h also should be removed, What do you think?

yronglin added inline comments.Jan 16 2023, 7:17 AM
libcxx/include/__expected/expected.h
67

Seems this namespace used to avoid ADL lookup?

$ ":" "RUN: at line 15"
note: command had no output on stdout or stderr
$ "clang-tidy-16" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/libcxx/test/libcxx/clang_tidy.sh.cpp" "--warnings-as-errors=*" "-header-filter=.*" "--checks=-*,libcpp-*" "--load=/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/libcxx/test/tools/clang_tidy_checks/libcxx-tidy.plugin" "--" "-nostdinc++" "-I" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1" "-I" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1" "-I" "/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/libcxx/test/support" "-std=c++2b" "-Werror" "-Wall" "-Wctad-maybe-unsupported" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" "-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-user-defined-literals" "-Wno-tautological-compare" "-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-D_LIBCPP_ENABLE_EXPERIMENTAL" "-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" "-Wuser-defined-warnings" "-fno-modules"
# command output:
/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:552:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
      __throw_bad_expected_access<_Err>(__union_.__unex_);
      ^
/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:559:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
      __throw_bad_expected_access<_Err>(__union_.__unex_);
      ^
/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:566:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
      __throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
      ^
/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:573:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
      __throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
      ^
/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:1167:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
      __throw_bad_expected_access<_Err>(__union_.__unex_);
      ^
/home/libcxx-builder/.buildkite-agent/builds/8ae0e84df61e-1/llvm-project/libcxx-ci/build/generic-cxx2b/include/c++/v1/__expected/expected.h:1173:7: error: ADL lookup [libcpp-robust-against-adl,-warnings-as-errors]
      __throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
      ^

# command stderr:
6 warnings generated.
Suppressed 48 warnings (48 NOLINT).
6 warnings treated as errors

error: command failed with exit status: 1
philnik added inline comments.Jan 16 2023, 9:29 AM
libcxx/include/__expected/expected.h
67

Yes, I think we should also remove __unexpected, but let's do that in another patch. You have to add std:: then calling __throw_bad_expected_access now to avoid the ADL lookup.

libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp
10 ↗(On Diff #488054)

I'm not super happy with just ignoring errors, since we might miss it if they change in a relevant way. OTOH having a lot of other errors littered in between also seems like a not-so-great thing. IDK, does anybody else have thoughts here?

libcxx/test/std/utilities/expected/expected.void/observers/error_or.pass.cpp
94

I thought in the diff I looked at test_default_template_arg() was never called at runtime. Ignore my comment.

yronglin updated this revision to Diff 489675.Jan 16 2023, 7:34 PM

Remove __expected namespace

yronglin marked an inline comment as done.Jan 16 2023, 7:34 PM
yronglin added inline comments.
libcxx/include/__expected/expected.h
67

+1

yronglin updated this revision to Diff 489832.Jan 17 2023, 8:20 AM
yronglin marked an inline comment as done.

Rebase

tcanens added inline comments.
libcxx/include/__expected/expected.h
867–871

Guaranteed elision into a potentially-overlapping subobject is unsettled (and it's not clear that it's implementable, given that the function is allowed to clobber the tail padding) - see https://github.com/itanium-cxx-abi/cxx-abi/issues/107.

yronglin marked an inline comment as done.Jan 18 2023, 9:56 AM
yronglin added inline comments.
libcxx/include/__expected/expected.h
867–871

Many Thanks for your reminder @tcanens ! Learned from this discussion, C++ standard still requires do direct initialization, but that impossible in current ABI, so, gcc says this code ( https://godbolt.org/z/K6edrdTG5) is ill-formed, but if we remove [[no_unique_address]], that works again. For the usability and robustness of std::expected, I think should we remove [[no_unique_address]] attribute until CWG2403 resolved this issue? Of course this will loses the benefit of [[no_unique_address]], and I don't have a strong opinion either way. I believe you have more experience than me. what do you all think?

philnik added inline comments.Jan 18 2023, 3:42 PM
libcxx/include/__expected/expected.h
867–871

We can't just apply [[no_unique_address]] later, since it would be an ABI break. We could do something like

union __union_t requires (is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>) {
  _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
  _LIBCPP_NO_UNIQUE_ADDRESS _Err __unex_;
  ...
};

union __union_t {
  _Tp __val_;
  _Err __unex_;
  ...
};

_LIBCPP_NO_UNIQUE_ADDRESS __union_t __union_;

That shouldn't be observable. @ldionne @huixie90 WDYT?

yronglin updated this revision to Diff 492383.Jan 26 2023, 3:50 AM
yronglin marked an inline comment as done.

rebase

yronglin retitled this revision from [libc++] Implement P2505R5(Monadic operations for std::expected) to [libc++] Implement P2505R5(Monadic operations for std::expected)..Feb 9 2023, 10:15 AM
yronglin updated this revision to Diff 497697.Feb 15 2023, 8:50 AM

Rebase and use [[no_unique_address]] if is_trivially_move_constructible_v<T> && is_trivially_move_constructible_v<E>

yronglin updated this revision to Diff 498593.Feb 18 2023, 8:52 AM

Try fix CI.

yronglin added inline comments.Feb 18 2023, 9:10 AM
libcxx/include/__expected/expected.h
867–871

@philnik @ldionne @huixie90 WDYT?

 struct __empty_t {};

 template <class _ValueType, class _ErrorType>
 union __union_t {
   constexpr __union_t() : __empty_() {}

   template <class _Func, class... _Args>
   _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
       std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
       : __val_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}

   template <class _Func, class... _Args>
   _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
       std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
       : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}

   _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
     requires(is_trivially_destructible_v<_ValueType> && is_trivially_destructible_v<_ErrorType>)
   = default;

   // the expected's destructor handles this
   _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
     requires(!is_trivially_destructible_v<_ValueType> || !is_trivially_destructible_v<_ErrorType>)
   {}

   __empty_t __empty_;
   _ValueType __val_;
   _ErrorType __unex_;
 };

 // use named union because [[no_unique_address]] cannot be applied to an unnamed union, 
 // also guaranteed elision into a potentially-overlapping subobject is unsettled (and 
 // it's not clear that it's implementable, given that the function is allowed to clobber 
 // the tail padding) - see https://github.com/itanium-cxx-abi/cxx-abi/issues/107.
 template <class _ValueType, class _ErrorType>
   requires(is_trivially_move_constructible_v<_ValueType> && is_trivially_move_constructible_v<_ErrorType>)
union __union_t<_ValueType, _ErrorType> {
   _LIBCPP_HIDE_FROM_ABI constexpr __union_t() : __empty_() {}

   template <class _Func, class... _Args>
   _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
       std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
       : __val_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}

   template <class _Func, class... _Args>
   _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
       std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
       : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}

   _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
     requires(is_trivially_destructible_v<_ValueType> && is_trivially_destructible_v<_ErrorType>)
   = default;

   // the expected's destructor handles this
   _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
     requires(!is_trivially_destructible_v<_ValueType> || !is_trivially_destructible_v<_ErrorType>)
   {}

   _LIBCPP_NO_UNIQUE_ADDRESS __empty_t __empty_;
   _LIBCPP_NO_UNIQUE_ADDRESS _ValueType __val_;
   _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
 };

 _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
 bool __has_val_;
yronglin updated this revision to Diff 499197.Feb 21 2023, 8:49 AM
yronglin updated this revision to Diff 499198.Feb 21 2023, 8:56 AM
yronglin marked an inline comment as done.

Update

philnik added inline comments.Mar 27 2023, 2:39 AM
libcxx/include/__expected/expected.h
867–871

Looks good from my side, but @ldionne and @huixie90 should also take a look.

yronglin updated this revision to Diff 510240.Apr 1 2023, 9:40 AM

Remove GCC XFAIL in transform/transform_error tests, because we have a workaround to avoid this issue.

Sorry sometimes patches get a bit lost in the review queue.
There are some things @ldionne should look at, I've asked him to have a look.

Sorry sometimes patches get a bit lost in the review queue.
There are some things @ldionne should look at, I've asked him to have a look.

Many thanks!

Sorry sometimes patches get a bit lost in the review queue.
There are some things @ldionne should look at, I've asked him to have a look.

Many thanks!

Thanks for the ping on Discord Mark. @yronglin Sorry for not seeing your pings, notifications on Phab don't allow subscribing to direct mentions so I don't see them, they get lost in other review traffic. Next time, don't hesitate to ping me on Discord (yeah I know it's a pain).

This almost LGTM but I have a question about constraints.

libcxx/include/__expected/expected.h
860

I don't quite understand why https://eel.is/c++draft/expected#object.monadic-2 says is_constructible_v<E, decltype(error())> but we're using is_copy_constructible_v<_Err>. I don't think those are equivalent, right? For example error() will return a non-const reference for this &-qualified function, but we're checking that is_constructible_v<E, E const&> instead of is_constructible_v<E, E&>? This is subtle, but if I'm right, we should add a test for this and then correct all the overloads below.

863–864

Here and below, qualification for this isn't required. We qualify function calls to avoid ADL, but this is not a function call.

libcxx/test/std/utilities/expected/expected.expected/monadic/transform.pass.cpp
13
libcxx/test/std/utilities/expected/expected.expected/monadic/transform_error.pass.cpp
13
libcxx/test/std/utilities/expected/expected.void/monadic/transform_error.pass.cpp
13
yronglin marked 2 inline comments as done.May 9 2023, 7:23 AM

Thanks for your review @ldionne !

libcxx/include/__expected/expected.h
860
863–864

Thanks, I'll remove that.

yronglin updated this revision to Diff 520755.EditedMay 9 2023, 10:40 AM
yronglin marked 2 inline comments as done.
  1. Address review comments
  2. Apply https://cplusplus.github.io/LWG/issue3877
  3. Apply https://cplusplus.github.io/LWG/lwg-defects.html#3866
  4. Add test for LWG 3877 and LWG 3866
ldionne accepted this revision.May 9 2023, 11:00 AM

LGTM w/ a few comments. Feel free to ship once those have been addressed, assuming the other reviewer's comments have been satisfied.

libcxx/docs/Status/Cxx2bPapers.csv
87

Can we mark the new LWG issues as resolved in libcxx/docs/Status/Cxx2bIssues.csv?

libcxx/include/__expected/expected.h
1410

Here too and a few below. I think you can grep for std::__is_expected!

Oh, and please make sure the CI is green.

Thanks a lot for the patch and sorry it went unnoticed for a while!

yronglin updated this revision to Diff 521012.May 10 2023, 8:50 AM

Fix tests

yronglin marked an inline comment as done.May 10 2023, 9:02 AM

Many thanks for your comments @ldionne !

libcxx/docs/Status/Cxx2bPapers.csv
87

+1

libcxx/include/__expected/expected.h
1063

So sorry, this code looks very ugly, it used to simplify the compile error diagnostic message(useful when I write the test case to check the static_assert message for LWG3866), do you have more good ideas for this?

before:

In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp:29:
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/expected:44:
/Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/__expected/expected.h:1433:19: error: static assertion failed due to requirement 'integral_constant<bool, false>::value': The result of f(error()) must be a valid template argument for unexpected
    static_assert(__valid_std_unexpected<_Gp>::value,
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp:50:7: note: in instantiation of function template specialization 'std::expected<void, int>::transform_error<std::unexpected<int> (&)(int &)>' requested here
    e.transform_error(return_unexpected); 
      ^
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp:29:
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/expected:44:
/Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/__expected/expected.h:954:17: error: static assertion failed due to requirement 'integral_constant<bool, false>::value': [expected.void.general] A program that instantiates expected<T, E> with a E that is not a valid argument for unexpected<E> is ill-formed
  static_assert(__valid_std_unexpected<_Err>::value,
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/__expected/expected.h:1436:14: note: in instantiation of template class 'std::expected<void, std::unexpected<int>>' requested here
      return expected<_Tp, _Gp>();
             ^
/Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp:50:7: note: in instantiation of function template specialization 'std::expected<void, int>::transform_error<std::unexpected<int> (&)(int &)>' requested here
    e.transform_error(return_unexpected); 
      ^
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp:29:
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/expected:44:
/Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/__expected/expected.h:1438:12: error: excess elements in struct initializer
    return expected<_Tp, _Gp>(__expected_construct_unexpected_from_invoke_tag{}, std::forward<_Func>(__f), error());
           ^                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp:50:7: note: in instantiation of function template specialization 'std::expected<void, int>::transform_error<std::unexpected<int> (&)(int &)>' requested here
    e.transform_error(return_unexpected); 
      ^
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp:29:
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/expected:44:

after:

In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp:29:
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/expected:44:
/Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/__expected/expected.h:814:19: error: static assertion failed due to requirement 'integral_constant<bool, false>::value': The result of f(error()) must be a valid template argument for unexpected
    static_assert(__valid_std_unexpected<_Gp>::value,
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp:45:7: note: in instantiation of function template specialization 'std::expected<int, int>::transform_error<std::unexpected<int> (&)(int)>' requested here
    e.transform_error(return_unexpected<int&>); // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}} The result of {{.*}} must be a valid template argument for unexpected}}
      ^
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp:29:
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/expected:44:
/Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/__expected/expected.h:814:19: error: static assertion failed due to requirement 'integral_constant<bool, false>::value': The result of f(error()) must be a valid template argument for unexpected
    static_assert(__valid_std_unexpected<_Gp>::value,
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp:46:7: note: in instantiation of function template specialization 'std::expected<int, int>::transform_error<int &(&)(int)>' requested here
    e.transform_error(return_no_object<int&>); // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}} The result of {{.*}} must be a valid template argument for unexpected}}
      ^
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp:29:
In file included from /Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/expected:44:
/Users/yrong/Developer/Toolchain/llvm/trunk/rel/include/c++/v1/__expected/expected.h:829:19: error: static assertion failed due to requirement 'integral_constant<bool, false>::value': The result of f(error()) must be a valid template argument for unexpected
    static_assert(__valid_std_unexpected<_Gp>::value,
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/yrong/Developer/Toolchain/llvm/trunk/llvm-project/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp:52:7: note: in instantiation of function template specialization 'std::expected<int, int>::transform_error<std::unexpected<int> (&)(int)>' requested here
    e.transform_error(return_unexpected<const int &>); // expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}} The result of {{.*}} must be a valid template argument for unexpected}}
      ^
yronglin marked 2 inline comments as done.May 10 2023, 9:04 AM
yronglin added inline comments.
libcxx/include/__expected/expected.h
1410

Sorry, I have missed that in previous commit, the qualifier removed now

ldionne added inline comments.May 10 2023, 9:08 AM
libcxx/include/__expected/expected.h
1063

I'm not sure I see how the "after" is better than the "before"? They look very similar, don't they?

yronglin marked 2 inline comments as done.May 10 2023, 9:13 AM
yronglin added inline comments.
libcxx/include/__expected/expected.h
1063

The "before" 1 case emit 3 errors, but "after" 1 case only emit 1 static assertion error.

yronglin marked an inline comment as done.May 10 2023, 9:20 AM
yronglin added inline comments.
libcxx/include/__expected/expected.h
1070

Redundant error diagnostic messages was generated by static_assert at the front of expected

static_assert(
      !is_reference_v<_Tp> &&
          !is_function_v<_Tp> &&
          !is_same_v<remove_cv_t<_Tp>, in_place_t> &&
          !is_same_v<remove_cv_t<_Tp>, unexpect_t> &&
          !__is_std_unexpected<remove_cv_t<_Tp>>::value &&
          __valid_std_unexpected<_Err>::value
      ,
      "[expected.object.general] A program that instantiates the definition of template expected<T, E> for a "
      "reference type, a function type, or for possibly cv-qualified types in_place_t, unexpect_t, or a "
      "specialization of unexpected for the T parameter is ill-formed. 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.");
ldionne accepted this revision.May 10 2023, 9:29 AM
ldionne added inline comments.
libcxx/include/__expected/expected.h
1063

IMO either way is acceptable. The real culprit is that Clang tries to keep compiling and that's really hard to work around that. For example with your current approach, the function will now return void and that could lead to other diagnostics.

yronglin updated this revision to Diff 521022.May 10 2023, 9:58 AM

Remove if constexpr in transform_error.

yronglin added inline comments.May 10 2023, 10:01 AM
libcxx/include/__expected/expected.h
1063

Thanks for your tips, I finally remove this approach.

ldionne accepted this revision.May 10 2023, 10:03 AM

Nice, let's ship this once CI is green!

yronglin updated this revision to Diff 521160.May 10 2023, 4:50 PM

Fix docs ci.

yronglin updated this revision to Diff 521291.May 11 2023, 6:27 AM

Try fix transform_error.mandates.verify.cpp

yronglin added a comment.EditedMay 12 2023, 6:13 AM

Could I land this patch now? all CIs are green, but the status of this differential still is NeedsReview @ldionne

@philnik @huixie90 Please let @yronglin know if you still have outstanding comments, but I think everything has been resolved.

@yronglin Please feel free to merge this in ~48h if no answer.

@philnik @huixie90 Please let @yronglin know if you still have outstanding comments, but I think everything has been resolved.

@yronglin Please feel free to merge this in ~48h if no answer.

Thanks!

philnik accepted this revision.May 16 2023, 8:43 AM

LGTM % comments.

libcxx/include/__expected/expected.h
1418

This shouldn't be required, since the requires clause causes the = defaulted to be preferred.

libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp
10 ↗(On Diff #488054)

I think I'd rather have a few more errors here than missing something important.

libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
11 ↗(On Diff #521294)

notes should normally be ignored. No need to specify this here.

yronglin updated this revision to Diff 522675.May 16 2023, 9:56 AM

Address comments.

yronglin marked 3 inline comments as done.May 16 2023, 10:01 AM

Thanks for your comments! @philnik

libcxx/include/__expected/expected.h
886

Also removed required here.

1418

+1, removed.

libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp
10 ↗(On Diff #488054)

Remove -verify-ignore-unexpected=error and checked more errors.

libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
11 ↗(On Diff #521294)

Remove -verify-ignore-unexpected=note

yronglin marked 3 inline comments as done.May 16 2023, 10:12 AM
yronglin added inline comments.
libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
11 ↗(On Diff #521294)

When I remove this flag, the command returned an error:

ld: file too small (length=0) file '/var/folders/pv/yzhm_ln121n085_jxzz652y80000gn/T/transform_error-b24073.o' for architecture arm64
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
philnik added inline comments.May 16 2023, 10:14 AM
libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
11 ↗(On Diff #521294)

This line shouldn't be required at all. The .verify already lets clang run in verify mode. It's probably confused because of the extra arguments.

yronglin updated this revision to Diff 522702.May 16 2023, 10:40 AM

Remove extra -Xclang -verify in tests

yronglin marked an inline comment as done.May 16 2023, 10:41 AM
yronglin added inline comments.
libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
11 ↗(On Diff #521294)

Thanks, removed!

This revision was not accepted when it landed; it landed in state Needs Review.May 17 2023, 10:06 AM
This revision was automatically updated to reflect the committed changes.
yronglin marked an inline comment as done.