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

Add RBMutex.TryRLock #138

Merged
merged 1 commit into from
Jul 13, 2024
Merged

Add RBMutex.TryRLock #138

merged 1 commit into from
Jul 13, 2024

Conversation

kkroo
Copy link
Contributor

@kkroo kkroo commented Jul 8, 2024

Attempt at adding RBMutex.TryRLock that tries to retrieve a reader lock token via the fast path or gives up returning nil.

Copy link
Owner

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

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

The overall approach seems fine. Tests for the new methods are missing.

rbmutex.go Show resolved Hide resolved
rbmutex.go Show resolved Hide resolved
func (mu *RBMutex) TryLock() bool {
return mu.rw.TryLock()
}
func (mu *RBMutex) fastTryRlock() (*rslot, *RToken) {
Copy link
Owner

Choose a reason for hiding this comment

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

The return types could be (int32, *RToken)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps ser, but i am not sure how -1 is added in the RLock path

@kkroo
Copy link
Contributor Author

kkroo commented Jul 10, 2024

nice library, thankz for the fast review.

@puzpuzpuz puzpuzpuz added the enhancement New feature or request label Jul 13, 2024
@puzpuzpuz
Copy link
Owner

Thanks for the contribution! I'll add the missing tests separately.

@puzpuzpuz puzpuzpuz merged commit f63979d into puzpuzpuz:main Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants