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

Eric Fiselier via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Nov 1 16:56:14 PDT 2022


EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

Please add a link to the paper in the change description. As well as a meaningful description for the change. 
Consider including any information you would want a reader in 5 years to have when they're puzzling over your code.

Also there's no reason to split this up into multiple headers. There are no deps we can cut from other headers by doing so, nor any circular dependency issues we're working around.

Another improvement that can be made is reducing the number of instantiations we perform in a lot of the metaprogramming. `_And`, `_Not`, and `_Or` should only be used when short circuiting is needed. In most cases here it isn't. Every single instantiation performed in expected is carried around by the compiler and in the AST for the entire translation unit. That's costly, lets avoid that cost.



================
Comment at: libcxx/include/__expected/bad_expected_access.h:39
+public:
+  _LIBCPP_HIDE_FROM_ABI const char* what() const noexcept override { return "bad access to std::expected"; }
+};
----------------
Why are we hiding this from the ABI? It's a vtable function, so I don't see how that's even possible?


================
Comment at: libcxx/include/__expected/expected.h:69
+template <class _Err, class _Arg>
+void __throw_bad_expected_access(_Arg&& __arg){
+#  ifndef _LIBCPP_NO_EXCEPTIONS
----------------
This needs to be hidden from the ABI. Perhaps always inlined or internal linkage. 


================
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>> >,                     //
----------------
ldionne wrote:
> Nitpick but you don't need those.
Just de morgan this entire thing?


================
Comment at: libcxx/include/__expected/expected.h:119
+  template <class _Gf>
+  static constexpr bool __can_assign_from_unexpected = _And< //
+      is_constructible<_Err, _Gf>,                           //
----------------
ldionne wrote:
> And this too, closer to the assignments.
Doesn't this have linkage?


================
Comment at: libcxx/include/__expected/expected.h:222
+      : __has_val_(__other.__has_val_) {
+    if (__has_val_) {
+      std::construct_at(std::addressof(__val_), std::move(__other.__val_));
----------------
Shouldn't `other.__has_val_` be 


================
Comment at: libcxx/include/__expected/expected.h:612-616
+  bool __has_val_;
+  union {
+    _Tp __val_;
+    _Err __unex_;
+  };
----------------
huixie90 wrote:
> ldionne wrote:
> > I would like to suggest swapping the union and the `bool` here. I think that will result in a more space efficient layout in most cases. You could confirm and change, or infirm and keep.
> I tried several examples and I could not find an example that the order of these two member affects the size of the class.
So the difference is where the padding goes. If the bool comes first the padding goes between the between the bool and the values, potentially causing the type to span over two cache lines or multiple registers.

Whereas if the bool is second, the padding will come after the bool at the end of the type. Which is significantly preferable. 


================
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));
+  }
----------------
Is there a reason to not just write the member access into the union? It would save function calls and instantiations, no?


================
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));
+  }
----------------
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.


================
Comment at: libcxx/include/__expected/expected.h:754
+
+#  if __cpp_concepts >= 202002
+  _LIBCPP_HIDE_FROM_ABI constexpr ~expected()
----------------
I don't think we can do this as it potentially changes the ABI of the type, no?

And that'll be ABI breaking between the two preprocessor branches.


================
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>> &&               //
----------------
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.


================
Comment at: libcxx/include/__expected/unexpected.h:51
+struct __valid_unexpected
+    : _And<                           //
+          is_object<_Tp>,             //
----------------
Don't write these metaprograms like this.

It's significantly less expensive in terms of compile time, and compiler memory to just turn the results into bools and operate on them that way.

These traits can't blow up when evaluated, so there's no reason to make them lazy.

In fact, we can do away with the entire class like ```template <class T> using __valid_unexpected = bool_constant<is_object_v<_Tp> && !is_array<_Tp>...>;`


================
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>
----------------
What's up with this change?


================
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>
----------------
Are these just merge conflicts / weird rebases? Because I don't understand this at all.


================
Comment at: libcxx/include/__type_traits/is_void.h:15
 
+#if !__has_builtin(__is_nothrow_constructible)
+#include <__type_traits/is_same.h>
----------------
What?


================
Comment at: libcxx/include/expected:11
+#ifndef _LIBCPP_EXEPECTED
+#define _LIBCPP_EXEPECTED
+
----------------
Why not define the things that are in this header, in this header?

Is there any reason to split them up? Like dependencies or the like?

Because every additional header comes at a cost, and so it needs to provide value to pay that cost.


================
Comment at: libcxx/test/std/utilities/expected/expected.void/ctor/ctor.unexpect_init_list.pass.cpp:55
+  template <class... Us>
+    requires std::is_constructible_v<std::tuple<Ts...>, Us&&...>
+  constexpr Data(std::initializer_list<int> il, Us&&... us) : vec_(il), tuple_(std::forward<Us>(us)...) {}
----------------
Is this requires clause necessary for the correctness of the test?


================
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>);
----------------
What does this test?


================
Comment at: libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp:58
+    try {
+      std::move(e).value();
+      assert(false);
----------------
We're missing test coverage on the `const&&` and `&` overloads of value(). 


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