Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 6, 2021

Lock annotations must be in the header, otherwise the will have limited or no effect

@ajtowns
Copy link
Contributor

ajtowns commented Jan 6, 2021

Concept ACK.

If you're moving things around in net.h to add lock annotations, might be worth moving NetEventsInterface to after the CNode definition, and making it virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_sendProcessing) = 0;.

Moving IsPeerAddrLocalGood and AdvertiseLocal later as well would allow removing the class CNode; forward declaration entirely.

It would be nice if there were some easy way to review pointer-becomes-reference changes.

@maflcko
Copy link
Member Author

maflcko commented Jan 6, 2021

It would be nice if there were some easy way to review pointer-becomes-reference changes.

Apart from --word-diff-regex=.?

SendMessages ...

Thanks, done

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 6, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

ACK fad2e1f

Nice tidy-up.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK fad2e1f
With the suggestions in the commit messages on how to best show the diffs, the review was quite straight-forward and painless. 👌

@ajtowns
Copy link
Contributor

ajtowns commented Jan 7, 2021

ACK fad2e1f

@maflcko
Copy link
Member Author

maflcko commented Jan 7, 2021

Rebased, should be trivial to re-ACK with git range-diff or from scratch

MarcoFalke added 2 commits January 7, 2021 09:40
Can be reviewed with --color-moved=dimmed-zebra --patience
Also, add lock annotation to SendMessages

Can be reviewed with "--word-diff-regex=."
@jnewbery
Copy link
Contributor

jnewbery commented Jan 7, 2021

utACK fa21068

@maflcko maflcko merged commit 42675e7 into bitcoin:master Jan 7, 2021
@maflcko maflcko deleted the 2101-netLock branch January 7, 2021 14:00
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 7, 2021
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 25, 2022
Summary:
```
Can be reviewed with --color-moved=dimmed-zebra --patience
```

This is a move only change, no change in behavior.

Partial backport of [[bitcoin/bitcoin#20864 | core#20864]]:
bitcoin/bitcoin@fa0a717

Depends on D10879.

Ref T1696.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Maniphest Tasks: T1696

Differential Revision: https://reviews.bitcoinabc.org/D10880
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 25, 2022
Summary:
```
Also, add lock annotation to SendMessages

Can be reviewed with "--word-diff-regex=."
```

Completes backport of [[bitcoin/bitcoin#20864 | core#20864]]:
bitcoin/bitcoin@fa21068

Depends on D10880.

Ref T1696.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Maniphest Tasks: T1696

Differential Revision: https://reviews.bitcoinabc.org/D10881
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
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.

5 participants