[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12
Hui via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Nov 2 05:32:47 PDT 2022
huixie90 marked 3 inline comments as done.
huixie90 added inline comments.
================
Comment at: libcxx/include/__expected/expected.h:104
+ is_constructible<_Err, _Gf>, //
+ _Not<is_constructible<_Tp, expected<_Up, _Gp>&> >, //
+ _Not<is_constructible<_Tp, expected<_Up, _Gp>> >, //
----------------
EricWF wrote:
> ldionne wrote:
> > Nitpick but you don't need those.
> Just de morgan this entire thing?
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
================
Comment at: libcxx/include/__expected/expected.h:593
+ static_assert(is_convertible_v<_Up, _Tp>, "argument has to be convertible to value_type");
+ return has_value() ? **this : static_cast<_Tp>(std::forward<_Up>(__v));
+ }
----------------
EricWF wrote:
> Is there a reason to not just write the member access into the union? It would save function calls and instantiations, no?
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
================
Comment at: libcxx/include/__expected/expected.h:600
+ static_assert(is_convertible_v<_Up, _Tp>, "argument has to be convertible to value_type");
+ return has_value() ? std::move(**this) : static_cast<_Tp>(std::forward<_Up>(__v));
+ }
----------------
EricWF wrote:
> 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.
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)
================
Comment at: libcxx/include/__expected/expected.h:75-89
+ static constexpr bool __can_convert =
+ is_constructible_v<_Tp, _Uf> && //
+ is_constructible_v<_Err, _Gf> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>> && //
----------------
EricWF wrote:
> ldionne wrote:
> > huixie90 wrote:
> > > philnik wrote:
> > > > ldionne wrote:
> > > > > That will evaluate these `is_constructible<...>` expressions in a short-circuiting fashion.
> > > > What exactly is the benefit of short-circuiting here? Is it required to satisfy the spec? If not, are you sure it's worth instantiating types and short-circuiting instead of using variables? I think instantiating types takes quite a bit longer, but I might be wrong on that.
> > > This is a good question. `is_xxx_v<T>` is no longer `is_xxx<T>::value` now. It is directly calling the compiler builtin. @ldionne , what do you think?
> > >
> > > Btw, I did a simple test with clang to compare the compile times between different approaches
> > >
> > > Program 1
> > > ```
> > > #include <type_traits>
> > > #include <utility>
> > >
> > > constexpr std::size_t template_length = 30000;
> > > template <std::size_t N> struct Foo {
> > > constexpr Foo(int)
> > > requires(N < template_length / 2)
> > > {}
> > > };
> > >
> > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > static_assert((std::is_constructible_v<Foo<Ns>, int> && ...));
> > > };
> > >
> > > template <std::size_t N> void generateTest() {
> > > test(std::make_index_sequence<N>{});
> > > }
> > >
> > > int main() { generateTest<template_length>(); }
> > >
> > > ```
> > > `time clang++ a.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.00secs`
> > >
> > > Program 2
> > > ```
> > > #include <type_traits>
> > > #include <utility>
> > >
> > > constexpr std::size_t template_length = 30000;
> > > template <std::size_t N> struct Foo {
> > > constexpr Foo(int)
> > > requires(N < template_length / 2)
> > > {}
> > > };
> > >
> > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > static_assert((std::is_constructible<Foo<Ns>, int>::value && ...));
> > > };
> > >
> > > template <std::size_t N> void generateTest() {
> > > test(std::make_index_sequence<N>{});
> > > }
> > >
> > > int main() { generateTest<template_length>(); }
> > >
> > > ```
> > > `time clang++ b.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.04secs`
> > >
> > > Program 3
> > > ```
> > > #include <type_traits>
> > > #include <utility>
> > >
> > > constexpr std::size_t template_length = 30000;
> > > template <std::size_t N> struct Foo {
> > > constexpr Foo(int)
> > > requires(N < template_length / 2)
> > > {}
> > > };
> > >
> > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > static_assert((std::_And<std::is_constructible<Foo<Ns>, int>...>::value));
> > > };
> > >
> > > template <std::size_t N> void generateTest() {
> > > test(std::make_index_sequence<N>{});
> > > }
> > >
> > > int main() { generateTest<template_length>(); }
> > >
> > > ```
> > > `time clang++ c.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `1.44 secs`
> > >
> > > @philnik So for this simple program, short circuiting (at the half position) is helpful
> > > What exactly is the benefit of short-circuiting here?
> >
> > The benefit is that we avoid doing work (defined after) if we can. The work I'm thinking about is: instantiating types and/or aliases, variable templates, etc. And that also includes calling the compiler builtins, because those are not free. Sure, that's much cheaper than implementing the same logic using pure C++, however builtins still require the compiler to go perform some non-trivial logic, and that has a cost. For example, under the hood, a builtin like `__is_constructible` still has to go and check whether `T(args...)` is well-formed, and that requires doing overload resolution on `T`'s constructor, potentially SFINAEing, etc. I actually don't *know* that that's how it's implemented, but from the outside it has to in order to give the right answer.
> >
> > So, TLDR: builtins are much cheaper to evaluate than the equivalent pure C++ functionality, but they still are not trivial, and that's what we save by short-circuiting. In comparison, short-circuiting requires us to implement a small constant number of additional templates (like `_Lazy`), whereas calling a single `__is_constructible` builtin could cause any number of types and functions to be instantiated.
> 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.
================
Comment at: libcxx/include/__expected/expected.h:75-89
+ static constexpr bool __can_convert =
+ is_constructible_v<_Tp, _Uf> && //
+ is_constructible_v<_Err, _Gf> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>> && //
----------------
huixie90 wrote:
> EricWF wrote:
> > ldionne wrote:
> > > huixie90 wrote:
> > > > philnik wrote:
> > > > > ldionne wrote:
> > > > > > That will evaluate these `is_constructible<...>` expressions in a short-circuiting fashion.
> > > > > What exactly is the benefit of short-circuiting here? Is it required to satisfy the spec? If not, are you sure it's worth instantiating types and short-circuiting instead of using variables? I think instantiating types takes quite a bit longer, but I might be wrong on that.
> > > > This is a good question. `is_xxx_v<T>` is no longer `is_xxx<T>::value` now. It is directly calling the compiler builtin. @ldionne , what do you think?
> > > >
> > > > Btw, I did a simple test with clang to compare the compile times between different approaches
> > > >
> > > > Program 1
> > > > ```
> > > > #include <type_traits>
> > > > #include <utility>
> > > >
> > > > constexpr std::size_t template_length = 30000;
> > > > template <std::size_t N> struct Foo {
> > > > constexpr Foo(int)
> > > > requires(N < template_length / 2)
> > > > {}
> > > > };
> > > >
> > > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > > static_assert((std::is_constructible_v<Foo<Ns>, int> && ...));
> > > > };
> > > >
> > > > template <std::size_t N> void generateTest() {
> > > > test(std::make_index_sequence<N>{});
> > > > }
> > > >
> > > > int main() { generateTest<template_length>(); }
> > > >
> > > > ```
> > > > `time clang++ a.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.00secs`
> > > >
> > > > Program 2
> > > > ```
> > > > #include <type_traits>
> > > > #include <utility>
> > > >
> > > > constexpr std::size_t template_length = 30000;
> > > > template <std::size_t N> struct Foo {
> > > > constexpr Foo(int)
> > > > requires(N < template_length / 2)
> > > > {}
> > > > };
> > > >
> > > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > > static_assert((std::is_constructible<Foo<Ns>, int>::value && ...));
> > > > };
> > > >
> > > > template <std::size_t N> void generateTest() {
> > > > test(std::make_index_sequence<N>{});
> > > > }
> > > >
> > > > int main() { generateTest<template_length>(); }
> > > >
> > > > ```
> > > > `time clang++ b.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.04secs`
> > > >
> > > > Program 3
> > > > ```
> > > > #include <type_traits>
> > > > #include <utility>
> > > >
> > > > constexpr std::size_t template_length = 30000;
> > > > template <std::size_t N> struct Foo {
> > > > constexpr Foo(int)
> > > > requires(N < template_length / 2)
> > > > {}
> > > > };
> > > >
> > > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > > static_assert((std::_And<std::is_constructible<Foo<Ns>, int>...>::value));
> > > > };
> > > >
> > > > template <std::size_t N> void generateTest() {
> > > > test(std::make_index_sequence<N>{});
> > > > }
> > > >
> > > > int main() { generateTest<template_length>(); }
> > > >
> > > > ```
> > > > `time clang++ c.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `1.44 secs`
> > > >
> > > > @philnik So for this simple program, short circuiting (at the half position) is helpful
> > > > What exactly is the benefit of short-circuiting here?
> > >
> > > The benefit is that we avoid doing work (defined after) if we can. The work I'm thinking about is: instantiating types and/or aliases, variable templates, etc. And that also includes calling the compiler builtins, because those are not free. Sure, that's much cheaper than implementing the same logic using pure C++, however builtins still require the compiler to go perform some non-trivial logic, and that has a cost. For example, under the hood, a builtin like `__is_constructible` still has to go and check whether `T(args...)` is well-formed, and that requires doing overload resolution on `T`'s constructor, potentially SFINAEing, etc. I actually don't *know* that that's how it's implemented, but from the outside it has to in order to give the right answer.
> > >
> > > So, TLDR: builtins are much cheaper to evaluate than the equivalent pure C++ functionality, but they still are not trivial, and that's what we save by short-circuiting. In comparison, short-circuiting requires us to implement a small constant number of additional templates (like `_Lazy`), whereas calling a single `__is_constructible` builtin could cause any number of types and functions to be instantiated.
> > 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.
================
Comment at: libcxx/include/__expected/expected.h:75-89
+ static constexpr bool __can_convert =
+ is_constructible_v<_Tp, _Uf> && //
+ is_constructible_v<_Err, _Gf> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, expected<_Up, _Gp>> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>&> && //
+ !is_constructible_v<_Tp, const expected<_Up, _Gp>> && //
----------------
huixie90 wrote:
> huixie90 wrote:
> > EricWF wrote:
> > > ldionne wrote:
> > > > huixie90 wrote:
> > > > > philnik wrote:
> > > > > > ldionne wrote:
> > > > > > > That will evaluate these `is_constructible<...>` expressions in a short-circuiting fashion.
> > > > > > What exactly is the benefit of short-circuiting here? Is it required to satisfy the spec? If not, are you sure it's worth instantiating types and short-circuiting instead of using variables? I think instantiating types takes quite a bit longer, but I might be wrong on that.
> > > > > This is a good question. `is_xxx_v<T>` is no longer `is_xxx<T>::value` now. It is directly calling the compiler builtin. @ldionne , what do you think?
> > > > >
> > > > > Btw, I did a simple test with clang to compare the compile times between different approaches
> > > > >
> > > > > Program 1
> > > > > ```
> > > > > #include <type_traits>
> > > > > #include <utility>
> > > > >
> > > > > constexpr std::size_t template_length = 30000;
> > > > > template <std::size_t N> struct Foo {
> > > > > constexpr Foo(int)
> > > > > requires(N < template_length / 2)
> > > > > {}
> > > > > };
> > > > >
> > > > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > > > static_assert((std::is_constructible_v<Foo<Ns>, int> && ...));
> > > > > };
> > > > >
> > > > > template <std::size_t N> void generateTest() {
> > > > > test(std::make_index_sequence<N>{});
> > > > > }
> > > > >
> > > > > int main() { generateTest<template_length>(); }
> > > > >
> > > > > ```
> > > > > `time clang++ a.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.00secs`
> > > > >
> > > > > Program 2
> > > > > ```
> > > > > #include <type_traits>
> > > > > #include <utility>
> > > > >
> > > > > constexpr std::size_t template_length = 30000;
> > > > > template <std::size_t N> struct Foo {
> > > > > constexpr Foo(int)
> > > > > requires(N < template_length / 2)
> > > > > {}
> > > > > };
> > > > >
> > > > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > > > static_assert((std::is_constructible<Foo<Ns>, int>::value && ...));
> > > > > };
> > > > >
> > > > > template <std::size_t N> void generateTest() {
> > > > > test(std::make_index_sequence<N>{});
> > > > > }
> > > > >
> > > > > int main() { generateTest<template_length>(); }
> > > > >
> > > > > ```
> > > > > `time clang++ b.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `2.04secs`
> > > > >
> > > > > Program 3
> > > > > ```
> > > > > #include <type_traits>
> > > > > #include <utility>
> > > > >
> > > > > constexpr std::size_t template_length = 30000;
> > > > > template <std::size_t N> struct Foo {
> > > > > constexpr Foo(int)
> > > > > requires(N < template_length / 2)
> > > > > {}
> > > > > };
> > > > >
> > > > > template <std::size_t... Ns> void test(std::index_sequence<Ns...>) {
> > > > > static_assert((std::_And<std::is_constructible<Foo<Ns>, int>...>::value));
> > > > > };
> > > > >
> > > > > template <std::size_t N> void generateTest() {
> > > > > test(std::make_index_sequence<N>{});
> > > > > }
> > > > >
> > > > > int main() { generateTest<template_length>(); }
> > > > >
> > > > > ```
> > > > > `time clang++ c.cpp -std=c++2b -stdlib=libc++ -fbracket-depth=50000` takes about `1.44 secs`
> > > > >
> > > > > @philnik So for this simple program, short circuiting (at the half position) is helpful
> > > > > What exactly is the benefit of short-circuiting here?
> > > >
> > > > The benefit is that we avoid doing work (defined after) if we can. The work I'm thinking about is: instantiating types and/or aliases, variable templates, etc. And that also includes calling the compiler builtins, because those are not free. Sure, that's much cheaper than implementing the same logic using pure C++, however builtins still require the compiler to go perform some non-trivial logic, and that has a cost. For example, under the hood, a builtin like `__is_constructible` still has to go and check whether `T(args...)` is well-formed, and that requires doing overload resolution on `T`'s constructor, potentially SFINAEing, etc. I actually don't *know* that that's how it's implemented, but from the outside it has to in order to give the right answer.
> > > >
> > > > So, TLDR: builtins are much cheaper to evaluate than the equivalent pure C++ functionality, but they still are not trivial, and that's what we save by short-circuiting. In comparison, short-circuiting requires us to implement a small constant number of additional templates (like `_Lazy`), whereas calling a single `__is_constructible` builtin could cause any number of types and functions to be instantiated.
> > > 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.
>
>
> 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)
================
Comment at: libcxx/include/__type_traits/is_member_pointer.h:15
+#if !__has_builtin(__is_nothrow_constructible)
+#include <__type_traits/is_member_function_pointer.h>
----------------
EricWF wrote:
> What's up with this change?
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.
================
Comment at: libcxx/include/__type_traits/is_scalar.h:16
#include <__type_traits/is_enum.h>
+#include <__type_traits/is_fundamental.h>
#include <__type_traits/is_member_pointer.h>
----------------
EricWF wrote:
> Are these just merge conflicts / weird rebases? Because I don't understand this at all.
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>
```
================
Comment at: libcxx/include/__type_traits/is_void.h:15
+#if !__has_builtin(__is_nothrow_constructible)
+#include <__type_traits/is_same.h>
----------------
EricWF wrote:
> What?
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
================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/value_or.mandates.verify.cpp:58
+ //expected-error-re@*:* {{{{(static_assert|static assertion)}} failed {{.*}}value_type has to be move constructible}}
+ //expected-error-re@*:* {{{{call to deleted constructor of}} {{.*}}}}
+ }
----------------
EricWF wrote:
> Is the deleted constructor not on one of the user-provided types?
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
================
Comment at: libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp:33
+ std::expected<void, int> e;
+ e.value();
+ static_assert(std::is_same_v<decltype(e.value()), void>);
----------------
EricWF wrote:
> What does this test?
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)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124516/new/
https://reviews.llvm.org/D124516
More information about the libcxx-commits
mailing list