Page MenuHomePhabricator

Merge #14444: Add compile time checking for cs_main locks which we assert at run time
ClosedPublic

Authored by nakihito on Oct 7 2019, 23:51.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCf15d3c8c03ed: Merge #14444: Add compile time checking for cs_main locks which we assert at…
Summary

0089905361 Add compile time checking for cs_main locks which we assert at run time (practicalswift)

Pull request description:

Assert locking requirements at compile-time (`EXCLUSIVE_LOCKS_REQUIRED(foo)`) instead of at run-time (`AssertLockHeld(…)`).

Tree-SHA512: f4965ebf4bb5dbf5e7ed738cacf82c0f6cd55134fb968860bf84a84e29806485617f223910bb8c5461213f1829b0137c64ba1f6d6a2008b3cac3bb3a28df9324

Backport of Core PR14444
https://github.com/bitcoin/bitcoin/pull/14444/

Test Plan
../configure CXX=clang++ CC=clang
make check

Run teamcity build-werror

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR14444
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7783
Build 13605: Bitcoin ABC Buildbot (legacy)
Build 13604: arc lint + arc unit

Event Timeline

nakihito created this revision.Oct 7 2019, 23:51
Owners added a reviewer: Restricted Owners Package.Oct 7 2019, 23:51
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 7 2019, 23:51
nakihito planned changes to this revision.Oct 7 2019, 23:51
nakihito requested review of this revision.Oct 7 2019, 23:52
nakihito edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Oct 8 2019, 15:58

2/3 of the PR are missing.

This revision now requires changes to proceed.Oct 8 2019, 15:58
nakihito requested review of this revision.EditedOct 9 2019, 01:04
nakihito edited the summary of this revision. (Show Details)

2/3 of the PR are missing.

The two lines changes were included in D4026. Edited summary to better clarify.
https://reviews.bitcoinabc.org/D4026#inline-25626
https://reviews.bitcoinabc.org/D4026#inline-25627

deadalnix requested changes to this revision.Oct 14 2019, 17:02

Why is that split this way? s there a reason this patch cannot be done by itself as it is in the PR?

Simple patches will help you get faster review. Moving code from a small patch to a large one is usually a losing strategy. In this case, now, you end up having to explain why thing are splet the way they are, which really isn't obvious.

This revision now requires changes to proceed.Oct 14 2019, 17:02
nakihito updated this revision to Diff 13569.Oct 16 2019, 01:50

Added changes to validation.cpp. Removed them from D4026.

nakihito planned changes to this revision.Oct 16 2019, 01:50
nakihito requested review of this revision.Oct 16 2019, 01:53
nakihito edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Oct 17 2019, 01:33

Actually run your test plan.

This revision now requires changes to proceed.Oct 17 2019, 01:33
nakihito updated this revision to Diff 13688.Thu, Oct 24, 17:49

Rebased.

nakihito planned changes to this revision.Thu, Oct 24, 17:49
nakihito requested review of this revision.Thu, Oct 24, 18:10
nakihito edited the test plan for this revision. (Show Details)

Adjusted test plan.

deadalnix accepted this revision.Fri, Oct 25, 01:42
This revision is now accepted and ready to land.Fri, Oct 25, 01:42