This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make future_error constructor standard-compliant
ClosedPublic

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

Details

Reviewers
curdeius
Group Reviewers
Restricted Project
Commits
rGd2cb198f25c8: [libc++] Make future_error constructor standard-compliant
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.

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
540

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

Quuxplusone added inline comments.
libcxx/include/future
530–539

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

1372–1373

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
26 ↗(On Diff #334074)

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
32–33

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
530–539

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?

libcxx/include/future
530–539

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
530–539

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.

539

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
23

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
32–33

Yes, implicit conversion to error_code.

ldionne requested changes to this revision.Oct 6 2021, 1:10 PM
ldionne added inline comments.
libcxx/include/future
539

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
208

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.

This revision now requires changes to proceed.Oct 6 2021, 1:10 PM
ldionne commandeered this revision.Sep 26 2023, 9:57 AM
ldionne edited reviewers, added: curdeius; removed: ldionne.

[Github PR transition cleanup]

Commandeering to finish.

Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2023, 9:57 AM
ldionne updated this revision to Diff 557366.Sep 26 2023, 11:05 AM

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

ldionne updated this revision to Diff 557367.Sep 26 2023, 11:06 AM
ldionne retitled this revision from [libc++] Make future_error constructor standard-compliant. to [libc++] Make future_error constructor standard-compliant.
ldionne edited the summary of this revision. (Show Details)

Update patch message.

ldionne added a subscriber: Restricted Project.Sep 26 2023, 11:14 AM

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 updated this revision to Diff 557460.Sep 28 2023, 12:03 PM

Adjust ignore_format.txt

ldionne updated this revision to Diff 557560.Oct 3 2023, 5:48 AM

XFAIL test on Windows

ldionne updated this revision to Diff 557599.Oct 4 2023, 1:45 PM

Fix XFAIL for real.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 5 2023, 6:12 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.