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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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.Mon, Oct 14, 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.Mon, Oct 14, 17:02
nakihito updated this revision to Diff 13569.Wed, Oct 16, 01:50

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

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

Actually run your test plan.

This revision now requires changes to proceed.Thu, Oct 17, 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