Page MenuHomePhabricator

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

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


Group Reviewers
Restricted Project

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

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

Quuxplusone added inline comments.

make_error_code should remain called via ADL; it's a customization point.
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());


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


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.


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.

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

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.


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.


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?


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).


Yes, implicit conversion to error_code.