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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 3827
Build 5727: Bitcoin ABC Teamcity Staging
Build 5726: arc lint + arc unit

Event Timeline

nakihito created this revision.Sep 21 2018, 04:24
Owners added a reviewer: Restricted Owners Package.Sep 21 2018, 04:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 21 2018, 04:24
nakihito updated this revision to Diff 4989.EditedSep 21 2018, 05:01

Fixed typo.

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
nakihito added inline comments.Sep 21 2018, 10:37
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.

jasonbcox added inline comments.Sep 21 2018, 17:07
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.

nakihito updated this revision to Diff 5172.Sep 29 2018, 06:30

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
nakihito updated this revision to Diff 5547.Oct 26 2018, 05:11

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
nakihito updated this revision to Diff 5730.Nov 10 2018, 21:16

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
nakihito abandoned this revision.Feb 4 2019, 22:43