Page MenuHomePhabricator

Seperated CBlockIndex class from chain.h into it blockindex.h and updated dependencies in other .h and .cpp files.
AbandonedPublic

Authored by nakihito on Aug 20 2018, 02:58.

Details

Reviewers
jasonbcox
deadalnix
schancel
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

Continues work on T380, dependent on D1659. Removed CBlockIndex class from chain.h into blockindex.h and updated other file dependencies.

Test Plan

Make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
SeperatingBlockIndexClass
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixsrc/blockindex.h:1CFMTCode style violation
Auto-Fixsrc/validation.cpp:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 3094
Build 4275: Bitcoin ABC Buildbot (legacy)
Build 4274: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox requested changes to this revision.Aug 20 2018, 21:50
jasonbcox added inline comments.
src/blockindex.h
206

Looks like some whitespace got deleted here. Not sure if that was intentional.

src/chain.h
21

Is this line necessary?

src/versionbits.h
9

Is chain.h still necessary here?

This revision now requires changes to proceed.Aug 20 2018, 21:50

Fixed white space typo and removed unnecessary chain.h include in versionbits.h.

Updated some unnecessary inlcudes.

deadalnix requested changes to this revision.Aug 21 2018, 13:03

It's not clear to me why many of the include change are required. Except that it seems like a good change.

src/blockindex.h
1 ↗(On Diff #4612)

The Bitcoin developers

221 ↗(On Diff #4612)

Keep

src/blockindexworkcomparator.h
8 ↗(On Diff #4612)

likestamp

src/chain.h
63 ↗(On Diff #4612)

This is the only place where CBlockIndex is not used by pointer and/or ref, so this would deserve to be commented on and have a plan to split this one as well so everything else in there can just use a forward declaration rather than an include.

src/checkpoints.cpp
7 ↗(On Diff #4612)

It doesn't seems like it is required.

src/init.cpp
14 ↗(On Diff #4612)

dito

src/rest.cpp
6 ↗(On Diff #4612)

dito

src/rpc/blockchain.cpp
10 ↗(On Diff #4612)

Why is it required now ?

src/test/blockencodings_tests.cpp
9 ↗(On Diff #4612)

???

This revision now requires changes to proceed.Aug 21 2018, 13:03

Will update this patch with recommended changes as soon as soon as I can.

src/blockindex.h
1 ↗(On Diff #4612)

Fixed.

221 ↗(On Diff #4612)

This is just a single newline between line 221 and 222, right? I will re-add this back in.

src/chain.h
63 ↗(On Diff #4612)

Doing this causes compile errors on my end.

src/checkpoints.cpp
7 ↗(On Diff #4612)

Ah, you're right. Will fix.

src/init.cpp
14 ↗(On Diff #4612)

Also, fixed. Although this might come back after CDiskBlockIndex is removed from chain.h in the next patch.

src/rest.cpp
6 ↗(On Diff #4612)

Also fixed.

src/rpc/blockchain.cpp
10 ↗(On Diff #4612)

Opps, this was a mistake I missed. Fixed.

src/test/blockencodings_tests.cpp
9 ↗(On Diff #4612)

This is actually necessary now because of some overly complicated include chains. The specific function call used is CheckProofOfWork().

Removed some unnecessary include statements and fixed some formating issues.

deadalnix requested changes to this revision.Sep 4 2018, 20:28
deadalnix added inline comments.
src/txdb.h
10 ↗(On Diff #4712)

Remove.

src/versionbits.h
8 ↗(On Diff #4712)

Forward declare CBlockIndex and remove.

This revision now requires changes to proceed.Sep 4 2018, 20:28
src/txdb.h
10 ↗(On Diff #4712)

Done.

src/versionbits.h
8 ↗(On Diff #4712)

Doing this requires adding an #include in the .cpp file. If that's preferable, just let me know.

src/versionbits.h
8 ↗(On Diff #4712)

Yes, it's preferable so that other code does not end up including blockindex.h as a side effect of including versionbits.h.

Removed unnecessary includes and added forward declaration as suggested.

deadalnix requested changes to this revision.Sep 10 2018, 14:39
deadalnix added inline comments.
src/chain.h
63 ↗(On Diff #4823)

Please add a comment as previously requested.

This revision now requires changes to proceed.Sep 10 2018, 14:39

Added comment signaling CDiskBlockIndex will be seperated in a subsequent diff.

deadalnix requested changes to this revision.Sep 12 2018, 00:38
deadalnix added inline comments.
src/chain.h
63 ↗(On Diff #4883)

That is not not what was asked.

This revision now requires changes to proceed.Sep 12 2018, 00:38
src/chain.h
63 ↗(On Diff #4883)

Sorry, I'm not quite sure what exactly you're asking for. Could you clarify what you want here for me?

Added TODO comment for CDiskBlockIndex.

Fixed comment formatting.

Fixed missing include statement.

src/chain.h
63 ↗(On Diff #4883)

*This is the only place where CBlockIndex is not used by pointer and/or ref*, so this would deserve to be commented on and have a plan to split this one as well so everything else in there can just use a forward declaration rather than an include.

What do you not understand in that comment ? If you do not understand something, you need to ask.

Updated to latest version of master.

jasonbcox requested changes to this revision.Nov 12 2018, 22:09

Trying to clear off the review queue temporarily. Will get back to review this later.

This revision now requires changes to proceed.Nov 12 2018, 22:09