Page MenuHomePhabricator

Moved CAddress from protocol.h and protocol.cpp into its own address.h and address.cpp files.
AbandonedPublic

Authored by nakihito on Sep 21 2018, 04:24.

Details

Reviewers
jasonbcox
deadalnix
schancel
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Maniphest Tasks
T407: protocol.h/cpp: Move CAddress to its own header/source files
Summary

Resolves T407. Removes CAddress from protocol.h and protocol.cpp into its own files: address.h and address.cpp.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
MoveCAddress
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3337
Build 4757: Bitcoin ABC Buildbot (legacy)
Build 4756: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Sep 21 2018, 04:24
jasonbcox requested changes to this revision.Sep 21 2018, 06:19
jasonbcox added inline comments.
src/address.cpp
9 ↗(On Diff #4989)

Instead of a TODO, I think you should do this diff first.

src/rpc/net.cpp
25 ↗(On Diff #4989)

The fact that this compiles is a fluke. You should always include from source files, and forward declarations may be used in headers where pointers or references only are used.

src/test/test_bitcoin_fuzzy.cpp
29 ↗(On Diff #4989)

This needs to be an include

This revision now requires changes to proceed.Sep 21 2018, 06:19
src/address.cpp
9 ↗(On Diff #4989)

I could do ServiceFlags first. I just started with CAddress since that was what the ticket requested and didn't want to start a change that wasn't asked for.

src/rpc/net.cpp
25 ↗(On Diff #4989)

Fixed.

src/test/test_bitcoin_fuzzy.cpp
29 ↗(On Diff #4989)

Fixed.

src/address.cpp
9 ↗(On Diff #4989)

Part of every ticket is identifying requirements. If you notice that doing other tasks before the ticket in question is necessary, then by all means do it. :)

btw, check out the #dev discussion regarding whether or not ServiceFlags should leave protocol.h. There's a case for leaving it in, but (in my opinion) it should only be done if protocol.h can be slimmed down and renamed to netprotocol.h or something that makes it obvious that it is net-code and not consensus-code.

Fixed accidental forward includes in a couple of .cpp files and replaced them with include statements. Also removed TODO comment in address.cpp because there was no consensus reached agreeing to the splitting of CServiceFlags.

deadalnix requested changes to this revision.Sep 29 2018, 09:16
deadalnix added inline comments.
src/Makefile.am
356 ↗(On Diff #5172)

Remove

src/net.h
37 ↗(On Diff #5172)

It seems like CAddress is used by value in there. Is this actually useful ?

src/seeder/bitcoin.cpp
3 ↗(On Diff #5172)

already included in the header

This revision now requires changes to proceed.Sep 29 2018, 09:16

Removed unnecessary include statements.

deadalnix requested changes to this revision.Nov 4 2018, 19:10
deadalnix added inline comments.
src/address.cpp
7 ↗(On Diff #5547)

It's already included in the header.

src/seeder/bitcoin.h
4 ↗(On Diff #5547)

Probably unnecessary here as the vector is passed by pointer.

24 ↗(On Diff #5547)

Vector is here.

This revision now requires changes to proceed.Nov 4 2018, 19:10

Removed unnecessary includes for address.h and untangled an include web.

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