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

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.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

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

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 edited the test plan for this revision. (Show Details)

Adjusted test plan.

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