refactor: Group and re-order CAddrMan members by access type #22025

1. 
   hebasto commented at 12:14 pm on May 23, 2021: member

   This PR is split from #19238 as all of its commits are trivial to review. The last commit is easy to review with git diff --color-moved=dimmed-zebra.

   Addressed the following comments from #19238:

   • https://github.com/bitcoin/bitcoin/pull/19238/commits/130b82ff3588d7207c928bda16b96e720e0b24cd#r550865131

   Can you consolidate all the private members and protected members to be next to each other? Multiple private and protected access specifiers make this harder to read than is necessary.

   • https://github.com/bitcoin/bitcoin/pull/19238/commits/130b82ff3588d7207c928bda16b96e720e0b24cd#r557271783

   Yeah, class declaration is easier to read if there is just one instance of public:, protected: and private: (in that order).

2. refactor: Do not expose CAddrMan members as protected without need 5cd7f8abe3
3. hebasto added the label Refactoring on May 23, 2021
4. 
   DrahtBot commented at 10:33 pm on May 23, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21940 (refactor: Mark CAddrMan::Select const by MarcoFalke)
   • #21129 (fuzz: check that ser+unser produces the same AddrMan by vasild)
   • #20233 (addrman: Make consistency checks a runtime option by jnewbery)
   • #19238 (refactor: Make CAddrMan::cs non-recursive by hebasto)
   • #18722 (addrman: improve performance by using more suitable containers by vasild)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. in src/addrman.h:760 in 6627b0bab4 outdated

   759+    friend class CAddrManCorrupted;
   760+    friend class CAddrManDeterministic;
   761+    friend class CAddrManSerializationMock;
   762+    friend class CAddrManTest;
   763+
   764 };
   

   ---

   ---

   

   vasild commented at 8:49 am on May 24, 2021:

   nit: this empty line can be removed

   ---

   

   hebasto commented at 8:53 am on May 24, 2021:

   Thanks! Done.
6. vasild approved
7. 
   vasild commented at 8:49 am on May 24, 2021: member

   ACK 6627b0bab44a6e8392611182fdec4c4a85ba10a0
8. hebasto force-pushed on May 24, 2021
9. vasild approved
10. 
    vasild commented at 9:04 am on May 24, 2021: member

    ACK 1b177a442a2e3749073cc189ddd404e74a902923
11. 
    jnewbery commented at 11:29 am on May 24, 2021: member

    ACK 1b177a442a2e3749073cc189ddd404e74a902923
12. move-only: Group and re-order CAddrMan members by access type

    Easy to verify with `git diff --color-moved=dimmed-zebra`.

    8caf60dbbe
13. in src/addrman.h:759 in 1b177a442a outdated

    754+    void Connected_(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
    755+
    756+    //! Update an entry's service bits.
    757+    void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
    758+
    759+    friend class CAddrManCorrupted;
    

    ---

    ---

    

    laanwj commented at 1:50 pm on May 24, 2021:

    Sorry but I’m not 100% sure that having this class know about all descendents in the tests is better than using protected.

    ---

    

    hebasto commented at 2:11 pm on May 24, 2021:

    Currently, one derived testing class has the friend specifier, others have access to protected members. Is combining both approaches better?

    ---

    

    laanwj commented at 2:16 pm on May 24, 2021:

    I’d say that if there is a choice, using protected is better than explicitly naming every descendant. Ideally you’d be able to add tests without having to mention them in the code to be tested. friend is like a last resort option for otherwise unrelated classes.

    ---

    

    hebasto commented at 2:17 pm on May 24, 2021:

    I see.

    ---

    

    hebasto commented at 4:36 pm on May 24, 2021:

    Thanks! Updated.
14. hebasto force-pushed on May 24, 2021
15. 
    hebasto commented at 4:36 pm on May 24, 2021: member

    Updated 1b177a442a2e3749073cc189ddd404e74a902923 -> 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab (pr22025.02 -> pr22025.03, diff):

    • addressed @laanwj’s comment
16. hebasto renamed this:
    refactor: Make CAddrMan protected members private ones
    refactor: Group and re-order CAddrMan members by access type
    on May 24, 2021
17. 
    jnewbery commented at 4:52 pm on May 24, 2021: member

    ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab
18. 
    practicalswift commented at 7:58 pm on May 24, 2021: contributor

    Concept ACK
19. 
    jarolrod commented at 4:32 am on May 26, 2021: member

    ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab

    Patch looks correct

20. vasild approved
21. 
    vasild commented at 1:32 pm on May 27, 2021: member

    ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab
22. 
    laanwj commented at 1:52 pm on May 27, 2021: member

    Code review ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab
23. laanwj merged this on May 27, 2021
24. laanwj closed this on May 27, 2021

    ---

25. hebasto deleted the branch on May 27, 2021
26. sidhujag referenced this in commit f8c7c25a8d on May 27, 2021
27. MarcoFalke referenced this in commit 3a2c84a6b5 on Jun 14, 2021
28. gwillen referenced this in commit af3617ff29 on Jun 1, 2022
29. DrahtBot locked this on Aug 16, 2022

Contributors
hebasto DrahtBot vasild jnewbery laanwj practicalswift jarolrod

Labels
Refactoring