sync: simplify and remove unused code from sync.h #25676
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
|
Reduce 4 of the `MaybeCheckNotHeld()` definitions to 2 by using a template. This also makes the function usable for other [BasicLockable](https://en.cppreference.com/w/cpp/named_req/BasicLockable) types.
The template parameter `typename Base = typename Mutex::UniqueLock` is not used, so remove it. Use internally defined type `Base` to avoid repetitions of `Mutex::UniqueLock`.
19fce03 to
f5b5293
Compare
|
|
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK f5b529369c8e5dee655ed559fd9583947188b18c. This makes nice simplifications and gets rid of some confusing shadowing, but I would suggest an alternative to the last commit.
I don't think the last commit "sync: rename the global UniqueLock to DebugLock" (f5b529369c8e5dee655ed559fd9583947188b18c) is good because it gets rid of the naming consistency between standard c++ thread primitives and our thread primitives. The standard c++ mutex class is called std::mutex while ours is called Mutex. The standard c++ recursive mutex class is called std::recursive_mutex while ours is called RecursiveMutex. Than standard c++ lock class is called std::unique_lock, so I think ours should be called UniqueLock, not DebugLock. It is true that the main features our lock class adds currently are related to debugging (though it also adds support for reverse locking), but we are not naming our custom classes after what features they add, otherwise we would be calling Mutex AnnotatedMutex etc. And I don't think it is a good idea to name our locking primitives after the particular features they add, because we should have flexibility to add and remove features from our locking primitives in the future.
Another benefit of dropping the last commit is that it would confine changes in this PR to sync.h and not change outside code in src/node/interfaces.cpp.
I do think one benefit of the last commit is that disambiguates global UniqueLock from AnnotatedMixin::UniqueLock. But I would fix this by renaming AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock. I think AnnotatedMixin::unique_lock name makes sense because it is a typedef for std::unique_lock, and would also be consistent with current UniqueLock::reverse_lock.
f5b5293 to
ea2bc6a
Compare
Use `MutexType` instead of `Mutex` for the template parameter of `UniqueLock` because there is already a class named `Mutex` and the naming overlap is confusing. `MutexType` is used elsewhere in `sync.h`.
Use `UniqueLock` directly. Type deduction works just fine from the first argument to the constructor of `UniqueLock`, so there is no need to repeat ```cpp UniqueLock<typename std::remove_reference<typename std::remove_pointer<decltype(cs)>::type>::type> ``` five times in the `LOCK` macros. Just `UniqueLock` suffices.
This avoids confusion with the global `UniqueLock` and the snake case is consistent with `UniqueLock::reverse_lock.
ea2bc6a to
75c3f9f
Compare
|
|
@vasild looks like you now also need to update the PR description? |
|
@fanquake, updated, thanks! |
aureleoules
left a comment
There was a problem hiding this comment.
ACK 75c3f9f - LGTM
I verified all commits compile and unit tests pass.
|
Post-merge ACK 75c3f9f Nice cleanup, especially with the final simplifications |
|
Post-merge ACK 75c3f9f |
75c3f9f sync: rename AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock (Vasil Dimov) 8d9ee8e sync: remove DebugLock alias template (Vasil Dimov) 4b2e167 sync: avoid confusing name overlap (Mutex) (Vasil Dimov) 9d7ae4b sync: remove unused template parameter from ::UniqueLock (Vasil Dimov) 11c190e sync: simplify MaybeCheckNotHeld() definitions by using a template (Vasil Dimov) Pull request description: Summary: * Reduce 4 of the `MaybeCheckNotHeld()` definitions to 2 by using a template. * Remove unused template parameter from `::UniqueLock`. * Use `MutexType` instead of `Mutex` for a template parameter name to avoid overlap/confusion with the `Mutex` class. * Rename `AnnotatedMixin::UniqueLock` to `AnnotatedMixin::unique_lock` to avoid overlap/confusion with the global `UniqueLock` and for consistency with `UniqueLock::reverse_lock`. The first commit `sync: simplify MaybeCheckNotHeld() definitions by using a template` is also part of bitcoin#25390 ACKs for top commit: aureleoules: ACK 75c3f9f - LGTM ryanofsky: Code review ACK 75c3f9f. Nice cleanups! Just suggested changes since last review: keeping UniqueLock name and fixing a missed rename in a code comment Tree-SHA512: ec261f6a444bdfe4f06e844b57b3606fdd9b2f842647cae15266d9729970d87585c808d482fbba0b31c33a4aa03527c36e282c92b28d9052711f75a7048c96f1
Summary:
MaybeCheckNotHeld()definitions to 2 by using a template.::UniqueLock.MutexTypeinstead ofMutexfor a template parameter name to avoid overlap/confusion with theMutexclass.AnnotatedMixin::UniqueLocktoAnnotatedMixin::unique_lockto avoid overlap/confusion with the globalUniqueLockand for consistency withUniqueLock::reverse_lock.The first commit
sync: simplify MaybeCheckNotHeld() definitions by using a templateis also part of #25390