refactor: Remove addrdb.h dependency from node.h #17297

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20191029-refactor-addrdb changing 7 files +23 −6

hebasto commented at 10:28 AM on October 29, 2019: member

node.h includes addrdb.h just for the sake of banmap_t type. This PR makes dependencies simpler and explicit.

~Also needless typedef has been removed from enum BanReason.~
refactor: Remove addrdb.h dependency from node.h f44abe4bed
hebasto commented at 10:30 AM on October 29, 2019: member

The touched code was originated by @jonasschnelli in #6310 (409bccfbf52b531b2a9d60ac2308f56223931a2e).
fanquake added the label Refactoring on Oct 29, 2019
practicalswift commented at 10:36 AM on October 29, 2019: contributor
Concept ACK

Thanks for cleaning this up.

Another header inconsistency that I think is worth cleaning up is the mismatch between core_io.h ⇄ core_write.cpp/core_read.cpp.

I think we should have:
- core_io.h ⇄ core_io.cpp, or
- core_read.h ⇄ core_read.cpp and core_write.h ⇄ core_write.cpp
This inconsistency makes it harder to reason about our header dependencies programmatically (such as the analysis performed in #16659) :)
hebasto commented at 10:49 AM on October 29, 2019: member

@practicalswift

This inconsistency makes it harder to reason about our header dependencies programmatically (such as the analysis performed in #16659) :)

I saw your #16659 (comment) ;)
DrahtBot commented at 12:01 PM on October 29, 2019: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
in src/addrdb.h:24 in c986288371 outdated
```
  25 | -} BanReason;
  26 | +enum BanReason {
  27 | +    BanReasonUnknown,
  28 | +    BanReasonNodeMisbehaving,
  29 | +    BanReasonManuallyAdded
  30 | +};
```
laanwj commented at 10:42 AM on October 30, 2019:

This is just asking to be a enum class.

hebasto commented at 8:01 PM on October 30, 2019:

It is not trivial as it involves (de)serialization, and requires some static_casts. Could we leave it out of this PR scope?

promag commented at 12:37 AM on November 2, 2019:

Please drop c9862883713ca3ee008444e132d961266331397f then, it's unrelated to the PR goal and it's already generating unnecessary "noise".

laanwj commented at 10:31 AM on November 2, 2019:

Yes, please make it fully move-only then.

hebasto commented at 1:06 PM on November 2, 2019:

Agree.
hebasto force-pushed on Nov 2, 2019
hebasto commented at 1:06 PM on November 2, 2019: member

Please drop c986288 then, it's unrelated to the PR goal and it's already generating unnecessary "noise".

Done.
laanwj commented at 3:14 PM on November 2, 2019: member

ACK f44abe4bed25a40145ab168adc1589f5df4146f3
practicalswift commented at 3:19 PM on November 2, 2019: contributor

ACK f44abe4bed25a40145ab168adc1589f5df4146f3
laanwj referenced this in commit c4b8dd2060 on Nov 4, 2019
laanwj merged this on Nov 4, 2019
laanwj closed this on Nov 4, 2019
hebasto deleted the branch on Nov 4, 2019
sidhujag referenced this in commit 15d32f7fab on Nov 7, 2019
jasonbcox referenced this in commit 9b0a206751 on Sep 25, 2020
sidhujag referenced this in commit e28607f6be on Nov 10, 2020
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

refactor: Remove addrdb.h dependency from node.h #17297

Conflicts