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

Tue Nov 15 11:34:37 PST 2022

ldionne added inline comments.

================
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"; }
+};
----------------
EricWF wrote:
> Why are we hiding this from the ABI? It's a vtable function, so I don't see how that's even possible?
We played around with it a bit: https://godbolt.org/z/fczzo8q5d

It seems like it doesn't make any difference. The compiler will still emit a virtual call whether it has the attribute always-inline (GCC) or abi-tag (Clang). However, since this method is in the vtable, it arguably can't change in any way it wants, so I think that using `_LIBCPP_HIDE_FROM_ABI` is misleading in that case. I think we should remove it from `what()` and from `~bad_expected_access()`.

I *think* we want to make it visibility=hidden, however, because we don't want people's shared libraries to start exporting this symbol.

Also, as a side note, the actual mangled name of this function doesn't matter too much, because it's almost always accessed as an index into the vtable of `bad_expected_access<void>`.

================
Comment at: libcxx/include/__expected/expected.h:754
+
+#  if __cpp_concepts >= 202002
+  _LIBCPP_HIDE_FROM_ABI constexpr ~expected()
----------------
EricWF wrote:
> 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.
I think this is a valid concern, and I seem to have missed that in my review. Thanks a lot for bringing this up.

IMO this means that we can only support compilers where `__cpp_concepts >= 202002`. If that means that `std::expected` doesn't work on Clang <= 15, I think that's fine. It'll be a bit annoying to add `XFAIL` annotations to the `expected` tests, but concretely it won't change anything for users because we release a matching Clang and libc++. And those XFAIL annotations will be cleaned up as our support window moves.

================
Comment at: libcxx/include/__expected/expected.h:310
+
+#  if __cpp_concepts >= 202002
+  _LIBCPP_HIDE_FROM_ABI constexpr ~expected()
----------------
jwakely wrote:
> 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
We don't often use feature-test macros to check for the availability of stuff. Instead, we check for compiler versions, which allows us to get rid of workarounds as the codebase evolves and we drop support for older compilers.

Yes, I know that's partly why feature-test macros were designed, but I think they provide more value to users than to us given our compiler support policy.

================
Comment at: libcxx/include/expected:26
+/*
+  Header <expected> synopsis
+
----------------
The synopsis usually comes before all the sub-headers (see for example `<optional>`).

================
Comment at: libcxx/include/expected:11
+#ifndef _LIBCPP_EXEPECTED
+#define _LIBCPP_EXEPECTED
+
----------------
EricWF wrote:
> 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.
Speaking with @huixie90 , he split them off originally to aid readability and because they are logically separated classes. That makes sense to me, and IMO the code is easier to understand in its current layout than if we put everything in the same 1300+ lines header.

We often split up much more granularly than this, so I'm not sure the cost of this split here is really meaningful.

================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/assert.arrow.pass.cpp:11
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0|12.0}}
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_ASSERTIONS=1
----------------
huixie90 wrote:
> ldionne wrote:
> > I don't think those are required. Can you try removing them and seeing if the CI fails?
> > 
> > If I'm not mistaken, that line would have been added to another test that you copy-pasted from because that other class had some back-deployment requirements. `expected` does not, because we decided not to put the exception class in the dylib.
> It turns out that after removing this, the test does fail on the Apple systems
> 
> https://reviews.llvm.org/harbormaster/unit/view/5402162/
This looks like a bug (in the test suite) to me. We should be defining `_LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED` whenever we override `__libcpp_verbose_abort` in the test suite, but we don't.

I think many existing tests are incorrectly using XFAIL.

You can either fix this in a prior patch, or re-add the XFAIL and we can fix all of them in a follow-up patch. I think this is the most reasonable approach to avoid blocking this review on a pre-existing issue.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124516