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

wayland-server: alive check for Weak #720

Merged
merged 2 commits into from
May 14, 2024

Conversation

cmeissl
Copy link
Collaborator

@cmeissl cmeissl commented May 5, 2024

In smithay we quite extensively use Weak as the key for caching some other resource. Cleaning up these resources
often requires checking if the key is still alive. Without an explicit function to check Weak for alive we have to resort to Weak::upgrade().is_ok(). An explicit function can save us from actually upgrading the resource itself, saving us quite some cpu time.

Copy link

codecov bot commented May 5, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 75.92%. Comparing base (1f6f4f5) to head (831899a).
Report is 1 commits behind head on master.

❗ Current head 831899a differs from pull request most recent head 85f69b8. Consider uploading reports for the commit 85f69b8 to get more accurate results

Files Patch % Lines
wayland-server/src/lib.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #720      +/-   ##
==========================================
+ Coverage   73.11%   75.92%   +2.81%     
==========================================
  Files          45       45              
  Lines        8183     8192       +9     
==========================================
+ Hits         5983     6220     +237     
+ Misses       2200     1972     -228     
Flag Coverage Δ
main 58.90% <44.44%> (-0.02%) ⬇️
test-- 81.00% <44.44%> (?)
test--server_system 64.64% <44.44%> (?)
test-client_system- 72.15% <44.44%> (?)
test-client_system-server_system 54.97% <44.44%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elinorbgr
Copy link
Member

Could you split out the rustfmt changes into another PR? Looks like the check-minimal CI is unhappy about something so I'd prefer we split this to figure out what's going on.

Other than that, LGTM 👍

@ids1024
Copy link
Member

ids1024 commented May 8, 2024

#722 should fix the CI failure if this is rebased.

this allows to check if a resource is still alive
without requiring to upgrade the whole resource.
these have shown up in a lot of traces in smithay
@cmeissl
Copy link
Collaborator Author

cmeissl commented May 8, 2024

sorry for late reply, ran out of time trying to fix the CI build. dropped the commit with the attempt to fix the CI as it is already fixed in master, thx :)

@cmeissl cmeissl requested review from ids1024 and elinorbgr May 10, 2024 21:58
@ids1024 ids1024 merged commit c1a6269 into Smithay:master May 14, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants