[libcxx-commits] [PATCH] D124516: [libc++] Implement `std::expected` P0323R12

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