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

Use of raw RWLock instead of RwLock #60576

Closed
hellow554 opened this issue May 6, 2019 · 5 comments
Closed

Use of raw RWLock instead of RwLock #60576

hellow554 opened this issue May 6, 2019 · 5 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@hellow554
Copy link
Contributor

hellow554 commented May 6, 2019

static HOOK_LOCK: RWLock = RWLock::new();
static mut HOOK: Hook = Hook::Default;

looking at these two lines of code is very frightening.
Is there a specific reason to use RWLock instead of RwLock? I would open a PR with either using RwLock or a comment describing why RWLock is needed here.

@rustbot modify labels: T-libs

@jonas-schievink jonas-schievink added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 6, 2019
@Lonami
Copy link
Contributor

Lonami commented May 6, 2019

Just another thought, isn't the naming itself a bit dangerous? RW vs. Rw seems like an easy typo to make. Perhaps renaming RW as RawRw could help?

@sfackler
Copy link
Member

sfackler commented May 6, 2019

RwLock can't be statically initialized, or at least couldn't be when that code was written. That'll change once the parking_lot PR lands.

@hellow554
Copy link
Contributor Author

hellow554 commented May 25, 2021

Hey @sfackler
has the parking_lot PR already landed? Can you maybe tell the PR number? I would like to give it a shot to clean this up.

@mati865
Copy link
Contributor

mati865 commented May 25, 2021

It was not merged: #56410 (comment)

@hellow554
Copy link
Contributor Author

hellow554 commented May 25, 2021

I will close this issue as I don't see that this can and will be addressed any time soon. Thank you for your answer mati :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants