scripted-diff: Small locking rename #11599
Conversation
|
Concept ACK. Drop |
Thanks, will wait for more feedback. I didn't rename CCriticalSection because it's used everywhere and changing it would make the diff 3 times as big (22 lines -> 73 lines). Also I think we will probably lean toward using non-recursive rather than recursive locks in new code, so CCriticalSection name should not matter as much going forward. I kept the C in CCriticalLock to be consistent with CCriticalSection. CCriticalLock is only used internally in sync.h so again it shouldn't have an impact on new code, and I didn't see a reason to break consistency within sync.h. |
|
utACK eec3e22 |
|
To me, calling it just "Mutex"/"Lock" implies this is what people should use for new mutexs/locks, but that isn't what we want because it doesn't support DEBUG_LOCKORDER. Indeed, we maybe could make it support DEBUG_LOCKORDER and then start migrating to it over our current recursive stuff, but for now I'd prefer to make it more clear that the new typedefs should be discouraged. |
|
Is DEBUG_LOCKORDER the only reason you want to discourage these? |
|
I suppose, I mean as long as its clear that its non-recursive and someone doesn't introduce a bug on accident cause they're not paying attention, DEBUG_LOCKORDER would be my only complaint. |
|
I'll just implement DEBUG_LOCKORDER for these. It should be pretty easy. |
|
If we're renaming them anyway, why not call them what they are? RecursiveMutex. RecursiveLock, Mutex, Lock? Do we need any special, project-local names for locking constructs at all or could we do with simply the c++11 naming? (I've wondered about this many times) I'm all for using non-recursive locks in new code, using recursive locks is usually discouraged nowadays. |
|
Indeed, after #11640, and because this is scripted, I'd be more than happy to see us drop the "CriticalSection" naming - no one uses that anymore... |
|
Agree with @laanwj – let's use the standard C++11 naming instead of project-local names for the locking constructs. Are there any good arguments for continuing with project-local names for the locking constructs? |
ryanofsky
force-pushed
the
pr/locksren
branch
from
eec3e22 to
244b2b9
Compare
ryanofsky
force-pushed
the
pr/locksren
branch
from
244b2b9 to
6cf6e51
Compare
ryanofsky
force-pushed
the
pr/locksren
branch
from
6cf6e51 to
aa68fa8
Compare
ryanofsky
force-pushed
the
pr/locksren
branch
2 times, most recently
from
3437332 to
8a02747
Compare
| The last travis run for this pull request was 87 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
|
Please get rid of the "Merge remote-tracking branch 'origin/pull/11640/head'" merge commit utACK otherwise |
Call sync.h primitives "locks" and "mutexes" instead of "blocks" and "waitable
critical sections" to match current coding conventions and c++11 standard
names.
This PR does not rename the "CCriticalSection" class (though this could be done
as a followup) because it is used everywhere and would swamp the other changes
in this PR. Plain mutexes should mostly be preferred instead of recursive
mutexes in new code anyway.
-BEGIN VERIFY SCRIPT-
set -x
set -e
ren() { git grep -l $1 | xargs sed -i s/$1/$2/; }
ren CCriticalBlock UniqueLock
ren CWaitableCriticalSection Mutex
ren CConditionVariable std::condition_variable
ren cs_GenesisWait g_genesis_wait_mutex
ren condvar_GenesisWait g_genesis_wait_cv
perl -0777 -pi -e 's/.*typedef.*condition_variable.*\n\n?//g' src/sync.h
-END VERIFY SCRIPT-
|
Needs rebase |
Rebased f21c529 -> 190bf62 (pr/locksren.8 -> pr/locksren.9) after #11599 merge. |
|
utACK 190bf62 |
190bf62 scripted-diff: Small locking rename (Russell Yanofsky) Pull request description: Call sync.h primitives "locks" and "mutexes" instead of "blocks" and "waitable critical sections" to match current coding conventions and c++11 standard names. This PR does not rename the "CCriticalSection" class (though this could be done as a followup) because it's used everywhere and would swamp the other changes in this PR. Plain mutexes should mostly be preferred instead of recursive mutexes in new code anyway. Tree-SHA512: 39b5b2be8f7a98227be8ab0648bdbb1b620944659bdc1eb9a15b0fcc0c930457fa0c03170cfedaeee0007ea716c526b31a8d84a86dd2333ce9d8bfabd773fe45
merge bitcoin#11640, bitcoin#11599, bitcoin#16112, bitcoin#16127, bitcoin#18635, bitcoin#19249: thread safety and locking improvements
8 participants
Call sync.h primitives "locks" and "mutexes" instead of "blocks" and "waitable critical sections" to match current coding conventions and c++11 standard names.
This PR does not rename the "CCriticalSection" class (though this could be done as a followup) because it's used everywhere and would swamp the other changes in this PR. Plain mutexes should mostly be preferred instead of recursive mutexes in new code anyway.