refactor: Replace RecursiveMutex with Mutex in timedata.cpp #19189
Conversation
|
Concept ACK. (nit: Could add a new commit to clang-format the whole file while touching it anyway?) |
|
Updated c2410ce -> cc5c0d2 (pr19189.01 -> pr19189.02, diff):
|
|
ACK cc5c0d2 , checked the second commit with --word-diff-regex=. --ignore-all-space -U0 🦉 Show signature and timestampSignature: Timestamp of file with hash |
| if (nOffset != 0 && abs64(nOffset) < 5 * 60) | ||
| fMatch = true; | ||
| for (const int64_t nOffset : vSorted) { | ||
| if (nOffset != 0 && abs64(nOffset) < 5 * 60) fMatch = true; |
There was a problem hiding this comment.
I know clang-format did it due to AllowShortIfStatementsOnASingleLine: true and so there is nothing to do in this PR, but I just want to mention that putting the if condition and body on one line like if (A) B;
- makes life in gdb more difficult (if one wants to set a breakpoint on B)
- makes backtraces less usefull (if it crashed on line 92, did it crash in
Aor inB?) - skews line based code coverage (if just
Ais executed and it wasfalsethen the code coverage may show the entire line 92 covered, giving the false impression thatBwas also executed).
There was a problem hiding this comment.
Lean to agree with you. Just want to preserve ACKs :)
There was a problem hiding this comment.
We only allow trivial statements in single line ifs, so it should always be clear whether it crashed in A or B.
Also, coverage reports reports generally include branch coverage.
| static RecursiveMutex cs_nTimeOffset; | ||
| static int64_t nTimeOffset GUARDED_BY(cs_nTimeOffset) = 0; | ||
| static Mutex g_timeoffset_mutex; | ||
| static int64_t nTimeOffset GUARDED_BY(g_timeoffset_mutex) = 0; |
There was a problem hiding this comment.
nit: s/nTimeOffset/g_time_offset/, feel free to ignore. I just wonder - you changed the name of one global variable why not the other too?
There was a problem hiding this comment.
I hope this commit is mutex related only :)
|
@sipa Mind reviewing this PR? |
Summary: > Only GetTimeOffset() and AddTimeData() functions lock this mutex. They do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the g_timeoffset_mutex could be a non-recursive mutex. This is a backport of [[bitcoin/bitcoin#19189 | core#19189]] Test Plan: ``` CC=clang CXX=clang++ cmake .. -DENABLE_SANITIZERS=thread -GNinja ninja all check check-functional ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9310
Only
GetTimeOffset()andAddTimeData()functions lock this mutex. They do not call itself recursively, and do not call each other either directly or indirectly. Therefore, theg_timeoffset_mutexcould be a non-recursive mutex.Related to #19180.