[net] remove an obscure macro used in CNode::copyStats #6207

pull Diapolo wants to merge 2 commits into bitcoin:master from Diapolo:net_macro changing 3 files +24 −27

Diapolo commented at 1:41 PM on May 31, 2015: none
- no code change
laanwj commented at 8:07 AM on June 1, 2015: member

Original code seems fine to me too. No strong opinion for or against this.
luke-jr commented at 4:35 AM on June 2, 2015: member

Seems like it'd be cleaner to just add a CNodeStats to CNode.
laanwj commented at 7:06 AM on June 2, 2015: member

@luke-jr That indeed sounds like a very clean solution. If indeed every field is copied, I don't see any immediate drawbacks.
sipa commented at 5:07 PM on June 2, 2015: member

ACK Luke's idea.
laanwj added the label Improvement on Jun 3, 2015
laanwj commented at 9:05 AM on June 6, 2015: member

@diapolo Are you going to do this?
Diapolo commented at 9:55 AM on June 6, 2015: none

I guess the proposed change could also requires some changes in rpcconsole and peertablemodel? Perhaps another dev takes a look at it, if we want to hurry... otherwise I'm going to look into this after the weekend.
[net] remove an obscure macro used in CNode::copyStats
- no code change
025b5f7496
squashme 84c6b85365
Diapolo commented at 10:39 AM on June 6, 2015: none

@sipa @laanwj Please see squashme commit, so I can be sure that is the direction you wanted.
jgarzik commented at 2:39 AM on June 7, 2015: contributor

meh. Code goes from clean to less clean.
Diapolo commented at 6:47 AM on June 8, 2015: none

I was asked to do so, but perhaps I did it the wrong way? We need to wait for @laanwj @luke-jr and @sipa to see their comments.
sipa commented at 10:06 AM on June 8, 2015: member

Not what I meant.

I meant removing the CNode values that are also in CNodeStats from CNode, and replacing them with a nodestats field. GetStats would then just return a copy of the nodestats field.
Diapolo commented at 11:51 AM on June 8, 2015: none

@sipa Well, nodestats is just the name of the CNodeStats attribute that you wanted me to add to the CNode class, right? So mine is currently named just stats. Only thing that differs is that I guess you wanted me to remove or rework CNode::copyStats() so that it returns a CNodeStats copy. Is this correct?
sipa commented at 11:59 AM on June 8, 2015: member

I meant moving all statistics into it, include nLastSend etc. Once you do that, CopyStats can just copy the nodestats field in its entirely, rather than copying all values from within CNode.
Diapolo commented at 12:22 PM on June 8, 2015: none

@sipa Right, as I've written I just wanted to be sure my change goes into the right direction (that's why I only included nServices).
laanwj commented at 7:54 AM on June 10, 2015: member

As this trivial issue appears to be controversial, I think it's not worth spending more time on. The original code is already clear in intent.
Diapolo closed this on Jun 12, 2015
Diapolo deleted the branch on Jun 12, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels