Page MenuHomePhabricator

refactor: replace CNode pointers by references within net_processing.{h,cpp}
ClosedPublic

Authored by Fabien on Thu, Nov 19, 16:55.

Details

Summary
This PR is inspired by a recent code review comment on a PR that
introduced new functions to the net_processing module. The point of the
discussion was basically that whenever we pass something not by value
(in the concrete example it was about CNode* and CConnman*) we should
either use

    a pointer (CType*) with null pointer check or
    a reference (CType&)

To keep things simple, this PR for a first approach

    only tackles CNode* pointers
    only within the net_processing module, i.e. no changes that would
need adaption in other modules
    keeps the names of the variables as they are

I'm aware that PRs like this are kind of a PITA to review, but I think
the code quality would increase if we get rid of pointers without
nullptr check -- bloating up the code by adding all the missing checks
would be the worse alternative, in my opinion.

Possible follow-up PRs, in case this is received well:

    replace CNode pointers by references for net module
    replace CConnman pointers by references for net_processing module
    ...

Backport of core PR19053.

Depends on D8469.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien requested review of this revision.Thu, Nov 19, 16:55
jasonbcox requested changes to this revision.Thu, Nov 19, 17:23
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/net_processing.cpp
488 ↗(On Diff #25878)

missing const

1201 ↗(On Diff #25878)

node can be const

This revision now requires changes to proceed.Thu, Nov 19, 17:23

Rebase and address feedback

This revision is now accepted and ready to land.Thu, Nov 19, 18:03