Skip to content

Conversation

@AdamBucior
Copy link
Contributor

@AdamBucior AdamBucior commented Apr 10, 2020

Resolves #51.

BTW without the checklist PRs feel so empty.

@AdamBucior AdamBucior requested a review from a team as a code owner April 10, 2020 15:50
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Apr 10, 2020
@StephanTLavavej
Copy link
Member

Thanks for the feature! I've exhaustively reviewed the product code but haven't really looked at the test coverage yet; I'll take a look at the tests during the next iteration.

AdamBucior and others added 9 commits April 11, 2020 10:36
Co-Authored-By: Stephan T. Lavavej <stl@nuwen.net>
Co-Authored-By: Stephan T. Lavavej <stl@nuwen.net>
Co-Authored-By: Stephan T. Lavavej <stl@nuwen.net>
Co-Authored-By: Stephan T. Lavavej <stl@nuwen.net>
Co-Authored-By: Stephan T. Lavavej <stl@nuwen.net>
Co-Authored-By: Stephan T. Lavavej <stl@nuwen.net>
@AdamBucior
Copy link
Contributor Author

BTW shouldn't _Invoker_ret and _Unforced be moved to <functional> if they're used only by std::function and bind()?

@AdamBucior
Copy link
Contributor Author

I don't understand why some of the x86 tests are failing.

@cbezault
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej StephanTLavavej self-assigned this May 13, 2020
The conditional noexcept on `_Invoker_ret::_Call` triggers VSO-1132186 "Weirdness with dependent template in noexcept-specifier".
@CaseyCarter
Copy link
Contributor

CaseyCarter commented May 28, 2020

I don't understand why some of the x86 tests are failing.

Two conditional noexcept bugs in Edge (the IntelliSense compiler based on EDG) that are C++17-or-later only. The reducing and filing was horrific, working around not too bad.

BTW shouldn't _Invoker_ret and _Unforced be moved to <functional> if they're used only by function and bind?

Yes, that makes sense to me.

@StephanTLavavej StephanTLavavej removed their assignment May 30, 2020
@AdamBucior
Copy link
Contributor Author

I think it's ready for initial review

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! My apologies for taking a month to get back to this. I found a couple of quiet hours to think hard about the changes, and everything appears to be in order. (Not easy to achieve, given the complexity of bind's machinery - I really appreciate your work here.) I'll push changes for some pre-existing/superficial syntactic things I noticed, and a bit more testing for conditionally noexcept bind(), and then I think this will be ready for final review and merge (to ship in VS 2019 16.8 Preview 1 if we can merge this week, otherwise Preview 2).

AdamBucior and others added 3 commits July 2, 2020 11:38
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
* Unwrapping a reference_wrapper is noexcept, but passing
     the unwrapped reference to the callable object
     might involve a possibly throwing conversion.
* Forwarding through a placeholder is noexcept, but passing
     the unbound argument to the callable object
     might involve a possibly throwing conversion.
* Nested bind() needs to sense whether the nested call might throw.

Mark PossiblyThrowingInt(int) as constexpr so we can construct
possibly_throwing_int during constant evaluation;
ref(possibly_throwing_int) is simpler than
declval<reference_wrapper<PossiblyThrowingInt>>().
@StephanTLavavej
Copy link
Member

Mini-changelog for more tests for conditionally noexcept bind():

  • Unwrapping a reference_wrapper is noexcept, but passing the unwrapped reference to the callable object might involve a possibly throwing conversion.
  • Forwarding through a placeholder is noexcept, but passing the unbound argument to the callable object might involve a possibly throwing conversion.
  • Nested bind() needs to sense whether the nested call might throw.

Mark PossiblyThrowingInt(int) as constexpr so we can construct possibly_throwing_int during constant evaluation; ref(possibly_throwing_int) is simpler than declval<reference_wrapper<PossiblyThrowingInt>>().

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a totally unrelated infrastructure issue with the arm/arm64 CI builds that we'll need to investigate, but I believe this PR is crystalline perfection. If a second reviewer can be equally convinced, I'll try to port this for Preview 1. 😺

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One style comment, but I'm happy to merge this either way.

@StephanTLavavej StephanTLavavej merged commit 98a10bf into microsoft:master Jul 3, 2020
@StephanTLavavej
Copy link
Member

Thanks again for implementing this C++20 feature (over 3% of the remaining features, by paper count!). This will ship in VS 2019 16.8 Preview 1. 😺 😸 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

P1065R2 constexpr INVOKE

5 participants