-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[significant_drop_tightening] Add #11125 test #11131
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
r? @xFrednet (rustbot has picked a reviewer for you, use r? to override) |
r% @blyxyas (xFrednet has picked a reviewer for you, use r% to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you simplify this? We only need sender_wakers
(or a Mutex<Vec<Waker>>
), the test doesn't need a lot of the original block to still be effective (e.g. the initial if
stmt. or SendValue
)
Is it really necessary to trim-down a self-contained use-case provided by a user of this lint? |
If #11128 fixed the initial problem with #11125 and the current CI (checking for the 100% faithful usecase) is successful, I think we can simplify it. Future-proofing each PR merged by adding the real usecase that caused the problem isn't something viable. |
I see. Thank you |
But looking at the current |
Is my understanding correct, that this PR only adds a test and that a simpler example of the same problem has been added in #11129? If this test doesn't check a different behavior or variant of one, I'd suggest closing the PR. If it's a variant, that checks something slightly different, it would be good to merge it. Merging it either way would not be wrong :) |
☔ The latest upstream changes (presumably #11161) made this pull request unmergeable. Please resolve the merge conflicts. |
Well, I think we'll do an exception this time. @c410-f3r, could you fix the conflicting files? 🤠 |
just a little mini-ping |
Thank you for the heads-up. Unfortunately, I don't have the motivation to continue pursuing this issue. |
Fix #11125
changelog: none