[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12
Hui via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Oct 22 13:16:59 PDT 2022
huixie90 marked 5 inline comments as done.
huixie90 added inline comments.
================
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>> && //
----------------
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
================
Comment at: libcxx/include/__expected/unexpected.h:70
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit unexpected(_Error&& __error) //
+ noexcept(is_nothrow_constructible_v<_Err, _Error>)
+ : __unex_(std::forward<_Error>(__error)) {}
----------------
philnik wrote:
> ldionne wrote:
> > We have discussed adding `noexcept` as an extension several times in the past, and I remember we've done it in a few places. This article explaining the Lakos principle is relevant: https://quuxplusone.github.io/blog/2018/04/25/the-lakos-rule/.
> >
> > 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.
> I think we actually have `noexcept` extensions in quite a few places. What exactly is the benefit of adding a new macro for this? I don't think we ever want to disable it, and I can't really imagine a scenario where you would be interested in `noexcept` extensions specifically. From what I can tell, adding a macro just makes the code harder to read.
perhaps noticing it is a libc++ extension (rather than what is specified in the standard) is already useful information?
alternatively we can always add comments.
================
Comment at: libcxx/include/__expected/unexpected.h:104
+
+ _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(unexpected& __x, unexpected& __y) //
+ noexcept(noexcept(__x.swap(__y)))
----------------
curdeius wrote:
> philnik wrote:
> > ldionne wrote:
> > > ldionne wrote:
> > > > Consider defining this one before `operator==` to make it closer to the member `swap`. That's also how it is in the spec, so bonus points for that.
> > > I am not sure how much I like this `//` pattern because it's becoming very prevalent. I _really_ like the way you've formatted this though, but I wish clang-format could get that result without so much help. @philnik any opinions on whether we can make `clang-format` do what we want here? Or maybe @curdeius, I think you've worked on `clang-format` a bunch lately?
> > What exactly are you asking for?
> > ```lang=c++
> > _LIBCPP_HIDE_FROM_ABI friend constexpr void swap(unexpected& __x, unexpected& __y) noexcept(noexcept(__x.swap(__y)))
> > requires is_swappable_v<_Err>
> > {
> > __x.swap(__y);
> > }
> > ```
> > seems just fine to me.
> Do you mean having (non-trivial) noexcept specifier on it's own line?
> There's no option like this right now, probably because it's not something you have a lot in non-generic code.
> It should be fairly easy to add an option to break before the noexcept specifier with possible values like the following: never (current behaviour), always (probably not very useful), only before non-trivial specifier i.e. with parentheses.
> Another option wind be to add a penalty for this, not sure if that suits the needs of libc++ though.
> So yeah, it would be necessary to clarify the desired formatting.
Thanks for the comments @philnik @curdeius
I think this particular one is not bad. But if you look at few lines above
```
template <class _Up, class... _Args>
requires is_constructible_v<_Err, initializer_list<_Up>&, _Args...>
_LIBCPP_HIDE_FROM_ABI constexpr explicit unexpected(in_place_t, initializer_list<_Up> __il, _Args&&... __args) //
noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>)
: __unex_(__il, std::forward<_Args>(__args)...) {}
```
If I remove `//`, clang-format would do
```
template <class _Up, class... _Args>
requires is_constructible_v<_Err, initializer_list<_Up>&, _Args...>
_LIBCPP_HIDE_FROM_ABI constexpr explicit unexpected(
in_place_t,
initializer_list<_Up> __il,
_Args&&... __args) noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>)
: __unex_(__il, std::forward<_Args>(__args)...) {}
```
It would separates the arguments into separate lines and put `noexcept` on the same line of the last argument, which looks a bit weird
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