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

[TRIVIAL] Remove all uses of the name scoped_lock #3309

Closed
wants to merge 1 commit into from

Conversation

HowardHinnant
Copy link
Contributor

  • scoped_lock is now a std name with subtly different semantics
    compared to lock_guard. Namely it can be used to lock 0 or
    more mutexes. This is valuable, but can also be accidentally
    used to lock 0 mutexes when 1 was intended, creating a
    run-time error.

    Therefore, if and when we use scoped_lock, extra care needs to
    be taken in reviewing that code to ensure it doesn't
    accidentally lock 0 mutexes when 1 was intended. To aid in
    such careful reviewing, the use of the name scoped_lock should
    be limited to those cases where the number of mutexes is not
    exactly one.

More in-depth rationale is here: https://stackoverflow.com/a/60172828/576911

*  scoped_lock is now a std name with subtly different semantics
   compared to lock_guard.  Namely it can be used to lock 0 or
   more mutexes.  This is valuable, but can also be accidentally
   used to lock 0 mutexes when 1 was intended, creating a
   run-time error.

   Therefore, if and when we use scoped_lock, extra care needs to
   be taken in reviewing that code to ensure it doesn't
   accidentally lock 0 mutexes when 1 was intended.  To aid in
   such careful reviewing, the use of the name scoped_lock should
   be limited to those cases where the number of mutexes is not
   exactly one.
@codecov-io
Copy link

Codecov Report

Merging #3309 into develop will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3309      +/-   ##
===========================================
- Coverage    70.62%   70.62%   -0.01%     
===========================================
  Files          676      676              
  Lines        54340    54340              
===========================================
- Hits         38376    38375       -1     
- Misses       15964    15965       +1     
Impacted Files Coverage Δ
src/ripple/core/impl/semaphore.h 100.00% <100.00%> (ø)
src/ripple/core/impl/LoadMonitor.cpp 90.27% <0.00%> (-1.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e3dc0e...571080a. Read the comment docs.

Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

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

👍

@HowardHinnant HowardHinnant added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 26, 2020
This was referenced Apr 7, 2020
@manojsdoshi manojsdoshi mentioned this pull request Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants