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

rwlock (and bswap8 because oops) #38

Merged
merged 3 commits into from
May 10, 2023
Merged

rwlock (and bswap8 because oops) #38

merged 3 commits into from
May 10, 2023

Conversation

lethalbit
Copy link
Member

This is just an initial draft of rwlock for #18, as it uses std::shared_mutex it's exclusive to C++17 or newer.

@dragonmux dragonmux added the enhancement New feature or request label Apr 20, 2023
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #38 (49a0195) into main (37e0e2f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
+ Coverage   96.25%   96.26%   +0.01%     
==========================================
  Files          52       52              
  Lines        3735     3745      +10     
  Branches      276      276              
==========================================
+ Hits         3595     3605      +10     
  Misses        128      128              
  Partials       12       12              
Impacted Files Coverage Δ
substrate/bits 100.00% <100.00%> (ø)
test/bits.cxx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

It's looking pretty good - the only changes we've noted (and we've only noted the first instance of each to keep the review light) are really stylistic consistency with the rest of the code base, and a note about how SUBSTRATE_NO_DISCARD() was intended to be used

We don't mind if you want to fix the points before merge or have us fix them after in a clean-up pass - just let us know which you'd like to do and we'll adjust the approval accordingly. Exciting to soon have a proper rwlock type available

substrate/rwlock Outdated Show resolved Hide resolved
substrate/rwlock Outdated Show resolved Hide resolved
substrate/rwlock Outdated Show resolved Hide resolved
@dragonmux dragonmux linked an issue Apr 26, 2023 that may be closed by this pull request
@lethalbit lethalbit requested a review from dragonmux May 10, 2023 11:43
@lethalbit lethalbit marked this pull request as ready for review May 10, 2023 11:43
Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing those items - we'll merge this just as soon as the builds complete

@dragonmux dragonmux merged commit 49a0195 into main May 10, 2023
54 of 165 checks passed
@dragonmux dragonmux deleted the aki/rwlock branch May 10, 2023 12:05
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.

[RFC]: Introduction of a rwlock wrapper type
2 participants