8000 Fix `static_mut_refs` warnings by OLEGSHA · Pull Request #2127 · glium/glium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix static_mut_refs warnings #2127

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

Merged
merged 2 commits into from
Oct 5, 2024
Merged

Conversation

OLEGSHA
Copy link
Contributor
@OLEGSHA OLEGSHA commented Sep 28, 2024

Fix for nightly check warnings discovered in #2126.

Turns out, most of static mut's are not needed in the first place, and the one that is necessary is a dirty hack regardless.

In tests/support/mod.rs:
- EVENT_LOOP_PROXY, WINDOW_RECEIVER, INIT_EVENT_LOOP, SEND_PROXY:
  `mut` is not necessary (anymore?)
- initialize_event_loop::<closure>::<closure>::WINDOWS:
  This is not an elegant solution, and refactoring half of mod.rs
  is beyond me. As such, just suppress the warning, it is a false
  positive here anyway.
Copy link
Collaborator
@est31 est31 left a comment

Choose a reason for hiding this comment

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

Could you make WINDOWS use a Mutex? This is the more future-proof way of addressing the lint.

@OLEGSHA
Copy link
Contributor Author
OLEGSHA commented Sep 29, 2024

I don't know how to make it work. If I wrap WINDOWS in a Mutex, then the window borrow on line 141 is no longer 'static because it originates from a locked mutex:

let mut mutex_lock = WINDOWS.lock().unwrap();
let window = &mutex_lock.as_mut().unwrap()[&key];
sender.send((window.into(), gl_config)).unwrap();
// `mutex_lock` dropped here while still borrowed

@OLEGSHA
Copy link
Contributor Author
OLEGSHA commented Sep 29, 2024

If I may express my opinion here, I think the use of a global variable here is the core issue. Ideally, the tests should be refactored to eliminate the need for it in the first place. I'm not yet ready to commit myself to this refactor, and I understand that no one else is available. I would then suggest that the awkward and annoying #[allow] band-aid is kept around so that this solution does not look prettier than it actually is.

In personal experience, if you want something to be temporary, make it ugly, so it's impossible to forget.

@OLEGSHA OLEGSHA requested a review from est31 October 4, 2024 19:06
justincredible added a commit to justincredible/glium that referenced this pull request Oct 5, 2024
After the refactoring, this is an immediate consequence of the observation from `glium`glium#2127.
@est31 est31 merged commit 9eec9e0 into glium:master Oct 5, 2024
7 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.

2 participants
0