Skip to content

Conversation

paveljanik
Copy link
Contributor

This is a followup to #8105, implements task 3 from it.

LOCK macro declares its variable criticalblock. When LOCK is used in nested block, again, the variable named criticalblock is used. When compiling with -Wshadow, this emits warning.

This change renames the variable criticalblock to criticalblockn, where n is ordinal number of the variable in the compiled file. Macro __COUNTER__ is used for this. It is supported by gcc (https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html), clang (http://clang.llvm.org/docs/LanguageExtensions.html) and Visual Studio (https://msdn.microsoft.com/en-us/library/b0084kay.aspx).

@sipa
Copy link
Member

sipa commented Aug 10, 2016

utACK 33d15a3

@paveljanik
Copy link
Contributor Author

More ACKs please.

@dcousens
Copy link
Contributor

utACK 33d15a3

@luke-jr
Copy link
Member

luke-jr commented Aug 29, 2016

I'd probably use LINE which is defined in the standards, but in practice it probably doesn't matter.

@paveljanik
Copy link
Contributor Author

paveljanik commented Aug 29, 2016

@luke-jr __LINE__ was thought of first, but @theuni raised "valid" (but not used yet) use-case (https://botbot.me/freenode/bitcoin-core-dev/2016-08-05/?msg=70857153&page=4):

std::mutex m1;
std::mutex m2;
LOCK(m1); LOCK(m2);

Of course you can use LOCK2 then instead, but...

Using __COUNTER__ allows even this use-case.

@theuni
Copy link
Member

theuni commented Aug 29, 2016

@paveljanik just to clarify, I was being snarky there. I don't think that's a real worry. In fact, 2 locks on the same line would almost certainly be wrong, as they should be std::lock()ed for safety anyway. So either way is fine with me.

utACK 33d15a3

@paveljanik
Copy link
Contributor Author

@theuni Ah, OK. Then I think we SHOULD use the standard __LINE__ even if __COUNTER__ works in all environments...

@sipa
Copy link
Member

sipa commented Aug 30, 2016

Using __LINE__ would result in a non-obvious error message, I guess. No strong opinion either way.

@paveljanik
Copy link
Contributor Author

So let's go with __COUNTER__ and we will see if it is problematic on some systems or if it is de-facto-standard.

@laanwj
Copy link
Member

laanwj commented Aug 31, 2016

Microsoft, gcc and clang implement __COUNTER__ so I'd say that's good enough.

We could even fall back to shadowing if it's not defined?

@paveljanik
Copy link
Contributor Author

Let's try it and see.

@laanwj
Copy link
Member

laanwj commented Sep 1, 2016

ACK, checked that bitcoind stays the same between 6e6ab2c and 33d15a3.

@laanwj laanwj merged commit 33d15a3 into bitcoin:master Sep 1, 2016
laanwj added a commit that referenced this pull request Sep 1, 2016
…ide LOCK

33d15a3 Do not shadow LOCK's criticalblock variable for LOCK inside LOCK (Pavel Janík)
LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 1, 2021
zkbot added a commit to zcash/zcash that referenced this pull request Apr 2, 2021
Bitcoin 0.14 locking PRs

These are locking changes from upstream (bitcoin core) release 0.14, oldest to newest (when merged to the master branch).

Each commit also includes a reference both to the PR and the upstream commit.

- bitcoin/bitcoin#8472
- bitcoin/bitcoin#8606
  - Excludes a lock move because we don't have bitcoin/bitcoin#7840 which this move was partially-reverting.
- bitcoin/bitcoin#9230
  - Only first commit (we don't have `LogTimestampStr` anymore).
- bitcoin/bitcoin#9243
  - Only the sixth commit, excluding `IsArgSet` locking (we haven't pulled that function in yet).
- bitcoin/bitcoin#9626
  - The cherry-picked commit does not match the upstream at all, but the resulting lock is useful.
- bitcoin/bitcoin#9679
- bitcoin/bitcoin#9227
- bitcoin/bitcoin#9698
  - Excludes change to `CConnman::PushMessage` in second commit (which we don't have yet).
- bitcoin/bitcoin#9708
- bitcoin/bitcoin#9771
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants