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

[Jonathan Wakely via Phabricator via libcxx-commits]

jwakely added inline comments.


================
Comment at: libcxx/include/__expected/expected.h:108
+      _Not<is_constructible<_Tp, const expected<_Up, _Gp>> >,               //
+      _Not<is_convertible<expected<_Up, _Gp>&, _Tp> >,                      //
+      _Not<is_convertible<expected<_Up, _Gp>&&, _Tp> >,                     //
----------------
ldionne wrote:
> I do not understand why the spec checks for `!is_convertible_v<expected<U, G>&, T>` after having already checked for `!is_constructible_v<T, expected<U, G>&>`. Shouldn't be `!is_constructible` imply `!is_convertible`?
> 
> The relevant sections are http://eel.is/c++draft/expected#object.cons-17.3 and http://eel.is/c++draft/expected#object.cons-17.7.
> 
> @jwakely Since you co-authored this paper, do you have any insight for me here? I assume I need to be taught something about C++, cause I'm certain there is a reason for this constraint.
> 
> Edit: @huixie90 just came up with an example where there's a distinction (which involves an ambiguous constructor call but non-ambiguous conversion), but I'm still curious to hear what's your take on this.
> @jwakely Since you co-authored this paper, do you have any insight for me here? I assume I need to be taught something about C++, cause I'm certain there is a reason for this constraint.

The pattern comes from `optional`'s converting ctors, see http://wg21.link/optional.ctor#27 and was added by https://wg21.link/lwg2756 which was originally done for `experimental::optional` in https://wg21.link/lwg2451 which links to the "Odd" example at https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4064.html#perfect_init as rationale.

IIRC it's there to prevent the ctor being a candidate for such odd types where direct-init would fail but copy-init would succeed.


================
Comment at: libcxx/include/__expected/expected.h:310
+
+#  if __cpp_concepts >= 202002
+  _LIBCPP_HIDE_FROM_ABI constexpr ~expected()
----------------
huixie90 wrote:
> ldionne wrote:
> > Which compiler(s) do not satisfy that?
> clang 14 does not support multiple destructors
> clang 14 does not support multiple destructors

And `__cpp_concepts >= 202002L` is the correct check for that, as specified by https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2493r0.html


================
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)) {}
----------------
ldionne wrote:
> huixie90 wrote:
> > 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.
> > I think we actually have `noexcept` extensions in quite a few places.
> 
> Yes we do, and I consider it a mistake that we haven't been marking them, since right now we don't have an easy way to know which `noexcept`s are spec-mandated and which ones are "we think it's okay to add it". That's useful information for us to have, and concretely there shouldn't be *that* many places where we add `noexcept` as an extension, so I don't think readability will be such a big concern.
> 
> It also calls out behavior that we should be testing in libc++ specific tests.
> 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.

IIRC MSVC just adds a comment saying `// strengthened` where they've added a non-required `noexcept`. That seems much lower overhead than a macro. Why use a macro if you never want it to be defined to anything except `noexcept`?

Libstdc++ doesn't bother, we just add it where it makes sense. It's just good QoI.

And if the standard says "*Throws*: Nothing." then that means it's **required** to never throw, so adding it is the right thing to do.

N.B. LEWG have decided to stop following the Lakos Rule, although that direction hasn't been approved at plenary yet. It's the right direction IMHO.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124516/new/

https://reviews.llvm.org/D124516