Page MenuHomePhabricator

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

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

Details

Reviewers
huixie90
philnik
ldionne
Mordante
var-const
Group Reviewers
Restricted Project
Summary

Diff Detail

Unit TestsFailed

TimeTest
610 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.std/utilities/expected/expected_expected/monadic::transform.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/expected/expected.expected/monadic/transform.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -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 -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -lc++experimental -L /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/test/std/utilities/expected/expected.expected/monadic/Output/transform.pass.cpp.dir/t.tmp.exe
610 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.std/utilities/expected/expected_expected/monadic::transform.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/expected/expected.expected/monadic/transform.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -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 -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -lc++experimental -L /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/test/std/utilities/expected/expected.expected/monadic/Output/transform.pass.cpp.dir/t.tmp.exe
610 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.std/utilities/expected/expected_expected/monadic::transform.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/expected/expected.expected/monadic/transform.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -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 -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -lc++experimental -L /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/test/std/utilities/expected/expected.expected/monadic/Output/transform.pass.cpp.dir/t.tmp.exe
520 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.std/utilities/expected/expected_expected/monadic::transform_error.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/expected/expected.expected/monadic/transform_error.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -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 -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -lc++experimental -L /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/test/std/utilities/expected/expected.expected/monadic/Output/transform_error.pass.cpp.dir/t.tmp.exe
520 mslibcxx CI GCC 12 / C++latest > llvm-libc++-shared-gcc-cfg-in.std/utilities/expected/expected_expected/monadic::transform_error.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-12 /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/expected/expected.expected/monadic/transform_error.pass.cpp -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-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-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wno-literal-suffix -Wno-user-defined-literals -Wno-tautological-compare -Wno-stringop-overread -Wno-stringop-overflow -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 -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -Wno-placement-new -Wno-class-memaccess -lc++experimental -L /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/ecc290c63a6b-1/llvm-project/libcxx-ci/build/generic-gcc/test/std/utilities/expected/expected.expected/monadic/Output/transform_error.pass.cpp.dir/t.tmp.exe
View Full Test Results (9 Failed)

Event Timeline

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

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
65–75

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
11

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
65–75

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
11

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
65–75

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
65–75

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
65–75

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
11

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
65–75

+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
943–947

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
943–947

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
943–947

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
943–947

@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 499198.Feb 21 2023, 8:56 AM
yronglin marked an inline comment as done.

Update

philnik added inline comments.Mon, Mar 27, 2:39 AM
libcxx/include/__expected/expected.h
943–947

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