Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<thread>: implement LWG-3788 #3130

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Conversation

strega-nil-ms
Copy link
Contributor

We implemented self-assign by stopping and joining; this was a bad idea, and there is now an LWG issue to say that we shouldn't do that.

See LWG-3788.

We implemented self-assign by stopping and joining; this was a bad idea,
and there is now an LWG issue to say that we shouldn't do that.
@strega-nil-ms strega-nil-ms requested a review from a team as a code owner September 26, 2022 15:58
@strega-nil-ms strega-nil-ms added the bug Something isn't working label Sep 26, 2022
@strega-nil-ms strega-nil-ms added this to Initial Review in Code Reviews via automation Sep 26, 2022
@strega-nil-ms strega-nil-ms moved this from Initial Review to Final Review in Code Reviews Sep 26, 2022
@strega-nil-ms strega-nil-ms changed the title <thread>: fix LWG-3788 <thread>: implement LWG-3788 Sep 26, 2022
@StephanTLavavej
Copy link
Member

@strega-nil-ms If possible, I strongly recommend writing a Proposed Resolution for LWG-3788 if you want it to be accepted this decade - issues without Proposed Resolutions tend to linger for years, while those with simple and uncontroversial Proposed Resolutions can be quickly accepted within a meeting or two.

@StephanTLavavej
Copy link
Member

This seems to be somewhat more daring than we usually are for not-yet-accepted LWG issues (that change the Standardese 180 degrees, instead of merely clarifying something ambiguous). In the past, we've been burned by speculatively implementing "obviously correct" fixes that later turned out to be controversial (e.g. future's blocking destructor, IIRC).

That said, I think it's reasonable to be slightly daring here, as the existing self-move-assign behavior is so wacky, being a no-op is much more intuitive, and (importantly) this is a hopefully obscure case in relatively new machinery, so the possible impact is small.

✅ No modules impact.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 26, 2022
@strega-nil-ms
Copy link
Contributor Author

strega-nil-ms commented Sep 29, 2022

Jonathan Wakely has put forward a resolution to this issue that the LWG mailing list seems happy about, which is in line with this implementation.

@StephanTLavavej StephanTLavavej self-assigned this Oct 11, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

I've pushed a merge with main to resolve a simple merge conflict with an updated WP citation in <thread> - this PR's change supersedes that.

@StephanTLavavej StephanTLavavej merged commit b29a16b into microsoft:main Oct 12, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 12, 2022
@StephanTLavavej
Copy link
Member

Thanks for fixing this wacky behavior in both the Standard and our implementation! 😹 😸 🎉

@strega-nil-ms strega-nil-ms deleted the lwg-3788 branch December 19, 2022 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants