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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added a comment.

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



================
Comment at: libcxx/include/future:505-514
+#if _LIBCPP_STD_VER <= 14
 public:
+#endif
     future_error(error_code __ec);
-#if _LIBCPP_STD_VERS > 14
-    explicit future_error(future_errc _Ev) : logic_error(), __ec_(make_error_code(_Ev)) {}
+
+public:
+#if _LIBCPP_STD_VER > 14
----------------
Quuxplusone wrote:
> curdeius wrote:
> > Quuxplusone wrote:
> > > `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
> > 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?
> 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.
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.


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


================
Comment at: libcxx/test/std/thread/futures/futures.future_error/code.pass.cpp:50
         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));
----------------
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).


================
Comment at: libcxx/test/std/thread/futures/futures.future_error/what.pass.cpp:33
     {
-        std::future_error f(std::make_error_code(std::future_errc::broken_promise));
+        std::future_error f(std::future_errc::broken_promise);
         LIBCPP_ASSERT(std::strcmp(f.what(), "The associated promise has been destructed prior "
----------------
Quuxplusone wrote:
> 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.
Yes, implicit conversion to `error_code`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99567