Page MenuHomePhabricator

[libc++] Make future_error constructor standard-compliant.
Needs ReviewPublic

Authored by curdeius on Mar 30 2021, 1:44 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

Found during D99515.
This patch tries not to change the behaviour for C++14 and below where there was no standard-compliant way for the user to construct a future_error.
From C++17 onwards, this patch makes future_error(future_errc) ctor explicit and make future_error(error_code) ctor private.
This will break the code of users that depend on this (non-standard) extension.

Diff Detail

Event Timeline

curdeius requested review of this revision.Mar 30 2021, 1:44 AM
curdeius created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 1:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius added inline comments.Mar 30 2021, 1:46 AM
libcxx/include/future
510

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

Quuxplusone added inline comments.
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 planned changes to this revision.Mar 30 2021, 7:44 AM
curdeius added inline comments.
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 added inline comments.Mar 30 2021, 7:53 AM
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 updated this revision to Diff 334390.Mar 31 2021, 2:58 AM
curdeius marked 5 inline comments as done.
  • Apply review comments.
  • Add future_error::__tag and rewrite ctors.
  • Test noexcept.
curdeius added a comment.EditedMar 31 2021, 3:01 AM

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.