[libc++] Make future_error constructor standard-compliant

[ldionne]

Summary

This patch removes the non compliant constructor of std::future_error
and adds the standards compliant constructor in C++17 instead.

Note that we can't support the constructor as an extension in all
standard modes because it uses delegating constructors, which require
C++11. We could in theory support the constructor as an extension in
C++11 and C++14 only, however I believe it is acceptable not to do that
since I expect the breakage from this patch will be minimal.

If it turns out that more code than we expect is broken by this, we can
reconsider that decision.

This was found during D99515.

[curdeius]

libcxx/include/future

510

Would it be better this way (probably with some specific error message)?

[• Quuxplusone]

libcxx/include/future

505–514

make_error_code should remain called via ADL; it's a customization point.
https://boostorg.github.io/outcome/motivation/plug_error_code.html
If your de-ADL'ed version passes the test suite, then we need more tests around this.

The pre-existing ctor sets up logic_error(__ec.message()) appropriately, so we want to find a way of duplicating that behavior. Also, I recommend adding a test for the message under test/libcxx/!

I'm suggesting that there be a private constructor from error_code (to avoid calling the user's make_error_code twice), but that it be tagged with a private tag to prevent people from stumbling into it accidentally. Your current patch leaves the existing ctor in place, but marks it private in C++17 and later; I think that's more likely to lead to confusion.

struct MyErrC { operator future_errc() const; operator error_code() const; };
auto err = std::future_error(MyErrC());

https://godbolt.org/z/rzs6K5a16

1353–1354

Maybe pull up the )); from the next line while you're here. Ditto line 1493.

libcxx/test/std/thread/futures/futures.future_error/ctor.fail.cpp

27

Per my suggestion above, I think "accidentally stumbling into a private constructor" is never a good thing. This specific error message indicates that we should do better.

libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp

33–34

Good; but how come these lines work in C++14 mode? Implicit conversion from future_errc to error_code, right?
In that case, your changes in test/std/thread/futures/futures.future_error/code.pass.cpp are too conservative; you can eliminate the #else condition and just run those tests in all modes.

[curdeius]

libcxx/include/future

505–514

Concerning make_error_code, isn't it a customisation point in the ctor of error_code only? Here we know that we call it with future_errc argument and we should call std-provided overload, no?

[• Quuxplusone]

libcxx/include/future

505–514

Ah, true. Okay, I retract that objection: it doesn't matter whether we qualify it or not. I have no strong opinion on whether it's better to consistently unqualify make_error_code everywhere it appears, or to consistently qualify every call except those that require ADL for correctness.

[curdeius]

• Apply review comments.
• Add future_error::__tag and rewrite ctors.
• Test noexcept.

[curdeius]

I'd love to hear your input on ABI breakage.

libcxx/include/future

505–514

I exposed __tag in all standard modes, because otherwise, I'd need to handle the construction of future_error in C++17 onwards and differently in previous standards.

509

What about ABI? Doing this change will certainly break it. What's the policy for that?
And why the ctor was in future.cpp and not inline? Is there something I should know about?

libcxx/test/std/thread/futures/futures.future_error/code.pass.cpp

50

I test noexcept in all standards modes, even if previously (pre-C++17) code() and what() were marked as throw() (which seems to be equivalent).

libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp

33–34

Yes, implicit conversion to error_code.

[ldionne]

libcxx/include/future

509

Indeed, this is an ABI break, and in fact the CI did catch it:

SYMBOL REMOVED: _ZNSt3__112future_errorC1ENS_10error_codeE
    {'is_defined': True, 'name': '_ZNSt3__112future_errorC1ENS_10error_codeE', 'type': 'FUNC'}
 
SYMBOL REMOVED: _ZNSt3__112future_errorC2ENS_10error_codeE
    {'is_defined': True, 'name': '_ZNSt3__112future_errorC2ENS_10error_codeE', 'type': 'FUNC'}

Perhaps we could define the constructor but make it private before C++17. That way, it is still part of the ABI but it can't be accessed by users. WDYT?

libcxx/src/future.cpp

205

This might not have been the case when you wrote that patch, however since then we always build libc++ with C++20. So we shouldn't need to use the __tag anywhere. In fact, I don't think we even need a __tag constructor at all, now.

[ldionne]

[Github PR transition cleanup]

Commandeering to finish.

[ldionne]

Rebase the patch, avoid ABI break, make 100% conforming (at the cost of potential breakage).

[ldionne]

Update patch message.

[ldionne]

Heads up for vendors. I'll ship this at the end of the week if nobody chimes in. If this causes problems after being landed, there's a few ways to try and reduce the scope of breakage.

[ldionne]

Adjust ignore_format.txt

[ldionne]

XFAIL test on Windows

[ldionne]

Fix XFAIL for real.

libcxx/include/future

 };
 
 template <> struct is_error_code_enum<future_errc> : public true_type { };
 error_code make_error_code(future_errc e) noexcept;
 error_condition make_error_condition(future_errc e) noexcept;
 
 const error_category& future_category() noexcept;
 
-class future_error
-    : public logic_error
+class future_error : public logic_error
 {
 public:
     future_error(error_code ec);  // exposition only
-    explicit future_error(future_errc); // C++17
+    explicit future_error(future_errc e); // C++17
+
     const error_code& code() const noexcept;
     const char*       what() const noexcept;
+
+private:
+    error_code ec_;             // exposition only
 };
 
 template <class R>
 class promise
 {
 public:
     promise();
     template <class Allocator>

 {
     return error_condition(static_cast<int>(__e), future_category());
 }
 
 class _LIBCPP_EXCEPTION_ABI _LIBCPP_AVAILABILITY_FUTURE_ERROR future_error
     : public logic_error
 {
     error_code __ec_;
 public:
-    future_error(error_code __ec);
-#if _LIBCPP_STD_VERS > 14
-    explicit future_error(future_errc _Ev) : logic_error(), __ec_(make_error_code(_Ev)) {}
+    struct __tag {
+        explicit __tag() = default;
+    };
+
+    explicit future_error(error_code __ec, __tag)
+        : logic_error(__ec.message()), __ec_(__ec) {}
+
+#if _LIBCPP_STD_VER > 14
+    explicit future_error(future_errc _Ev)
+        : future_error(make_error_code(_Ev), __tag()) {}
+#else
+    future_error(error_code __ec) : logic_error(__ec.message()), __ec_(__ec) {}
 #endif
+
     _LIBCPP_INLINE_VISIBILITY
     const error_code& code() const _NOEXCEPT {return __ec_;}
 
     future_error(const future_error&) _NOEXCEPT = default;
     virtual ~future_error() _NOEXCEPT;
 };
 
 _LIBCPP_NORETURN inline _LIBCPP_INLINE_VISIBILITY
 #ifndef _LIBCPP_NO_EXCEPTIONS
 _LIBCPP_AVAILABILITY_FUTURE_ERROR
 #endif
 void __throw_future_error(future_errc _Ev)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    throw future_error(make_error_code(_Ev));
+    throw future_error(make_error_code(_Ev), future_error::__tag());
 #else
     ((void)_Ev);
     _VSTD::abort();
 #endif
 }
 
 class _LIBCPP_TYPE_VIS _LIBCPP_AVAILABILITY_FUTURE __assoc_sub_state
     : public __shared_count
promise<_Rp>::promise(allocator_arg_t, const _Alloc& __a0)
     __state_ = _VSTD::addressof(*__hold.release());
 }
 
 template <class _Rp>
 promise<_Rp>::~promise()
 {
     if (__state_)
     {
         if (!__state_->__has_value() && __state_->use_count() > 1)
-            __state_->set_exception(make_exception_ptr(
-                      future_error(make_error_code(future_errc::broken_promise))
-                                                      ));
+              future_error(make_error_code(future_errc::broken_promise),
+                           future_error::__tag())));
         __state_->__release_shared();
     }
 }
 
 template <class _Rp>
 future<_Rp>
 promise<_Rp>::get_future()
 {

 }
 
 template <class _Rp>
 promise<_Rp&>::~promise()
 {
     if (__state_)
     {
         if (!__state_->__has_value() && __state_->use_count() > 1)
-            __state_->set_exception(make_exception_ptr(
-                      future_error(make_error_code(future_errc::broken_promise))
-                                                      ));
+              future_error(make_error_code(future_errc::broken_promise),
+                           future_error::__tag())));
         __state_->__release_shared();
     }
 }
 
 template <class _Rp>
 future<_Rp&>
 promise<_Rp&>::get_future()
 {

libcxx/src/future.cpp

 
 const error_category&
 future_category() noexcept
 {
     static __future_error_category __f;
     return __f;
 }
 
-future_error::future_error(error_code __ec)
-    : logic_error(__ec.message()),
-      __ec_(__ec)
-{
-}
-
 future_error::~future_error() noexcept
 {
 }
 
 void
 __assoc_sub_state::__on_zero_shared() noexcept
 {
     delete this;

 }
 
 promise<void>::~promise()
 {
     if (__state_)
     {
 #ifndef _LIBCPP_NO_EXCEPTIONS
         if (!__state_->__has_value() && __state_->use_count() > 1)
-            __state_->set_exception(make_exception_ptr(
-                      future_error(make_error_code(future_errc::broken_promise))
-                                                      ));
+              future_error(make_error_code(future_errc::broken_promise),
+                           future_error::__tag())));
 #endif // _LIBCPP_NO_EXCEPTIONS
         __state_->__release_shared();
     }
 }
 
 future<void>
 promise<void>::get_future()
 {

libcxx/test/std/thread/futures/futures.future_error/code.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: libcpp-has-no-threads
 
 // <future>
 
 // class future_error
-//     future_error(error_code __ec);  // exposition only
-//     explicit future_error(future_errc _Ev) : __ec_(make_error_code(_Ev)) {} // C++17
+//     future_error(error_code ec);  // exposition only
+//     explicit future_error(future_errc e); // C++17
 
-// const error_code& code() const throw();
+// const error_code& code() const noexcept;
 
 #include <future>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
+#if TEST_STD_VER <= 14
     {
         std::error_code ec = std::make_error_code(std::future_errc::broken_promise);
         std::future_error f(ec);
         assert(f.code() == ec);
     }
     {
         std::error_code ec = std::make_error_code(std::future_errc::future_already_retrieved);
         std::future_error f(ec);
         assert(f.code() == ec);
     }
     {
         std::error_code ec = std::make_error_code(std::future_errc::promise_already_satisfied);
         std::future_error f(ec);
         assert(f.code() == ec);
     }
     {
         std::error_code ec = std::make_error_code(std::future_errc::no_state);
         std::future_error f(ec);
         assert(f.code() == ec);
     }
-#if TEST_STD_VER > 14
+#endif
     {
         std::future_error f(std::future_errc::broken_promise);
+        ASSERT_NOEXCEPT(f.code());
         assert(f.code() == std::make_error_code(std::future_errc::broken_promise));
     }
     {
         std::future_error f(std::future_errc::no_state);
         assert(f.code() == std::make_error_code(std::future_errc::no_state));
     }
-#endif
 
   return 0;
 }

libcxx/test/std/thread/futures/futures.future_error/ctor.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: c++03, c++11, c++14
+// UNSUPPORTED: libcpp-has-no-threads
+
+// <future>
+
+// class future_error
+//     explicit future_error(future_errc e); // C++17
+
+#include <future>
+
+int main(int, char**) {
+  auto test_explicit = [](std::future_errc e) -> std::future_error {
+    return {e};
+    // expected-error@-1 {{chosen constructor is explicit in copy-initialization}}
+  };
+
+  std::error_code ec = std::make_error_code(std::future_errc::no_state);
+  std::future_error f(ec); // expected-error {{no matching constructor for initialization of 'std::future_error'}}
+
+  return 0;
+}

libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp

 // XFAIL: with_system_cxx_lib=macosx10.11
 // XFAIL: with_system_cxx_lib=macosx10.10
 // XFAIL: with_system_cxx_lib=macosx10.9
 
 // <future>
 
 // class future_error
 
-// const char* what() const throw();
+// const char* what() const noexcept;
 
 #include <future>
 #include <cstring>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
     {
-        std::future_error f(std::make_error_code(std::future_errc::broken_promise));
+        std::future_error f(std::future_errc::broken_promise);
+        ASSERT_NOEXCEPT(f.what());
         LIBCPP_ASSERT(std::strcmp(f.what(), "The associated promise has been destructed prior "
                       "to the associated state becoming ready.") == 0);
     }
     {
-        std::future_error f(std::make_error_code(std::future_errc::future_already_retrieved));
+        std::future_error f(std::future_errc::future_already_retrieved);
         LIBCPP_ASSERT(std::strcmp(f.what(), "The future has already been retrieved from "
                       "the promise or packaged_task.") == 0);
     }
     {
-        std::future_error f(std::make_error_code(std::future_errc::promise_already_satisfied));
+        std::future_error f(std::future_errc::promise_already_satisfied);
         LIBCPP_ASSERT(std::strcmp(f.what(), "The state of the promise has already been set.") == 0);
     }
     {
-        std::future_error f(std::make_error_code(std::future_errc::no_state));
+        std::future_error f(std::future_errc::no_state);
         LIBCPP_ASSERT(std::strcmp(f.what(), "Operation not permitted on an object without "
                       "an associated state.") == 0);
     }
 
   return 0;
 }