[libcxx-commits] [PATCH] D99567: [libc++] Make future_error constructor standard-compliant.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/future:512
+#if _LIBCPP_STD_VER > 14
+    explicit future_error(future_errc _Ev)
+        : future_error(_VSTD::make_error_code(_Ev)) {}
----------------
curdeius wrote:
> 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?
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?


================
Comment at: libcxx/src/future.cpp:205
+          __state_->set_exception(make_exception_ptr(
+              future_error(make_error_code(future_errc::broken_promise),
+                           future_error::__tag())));
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99567