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

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:210523-cam changing 1 files +132 −134
  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:

    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.

    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. hebasto cross-referenced this on May 23, 2021 from issue refactor: Make CAddrMan::cs non-recursive by hebasto
  5. DrahtBot commented at 10:33 PM on May 23, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  6. DrahtBot cross-referenced this on May 23, 2021 from issue refactor: Mark CAddrMan::Select and GetAddr const by MarcoFalke
  7. DrahtBot cross-referenced this on May 24, 2021 from issue fuzz: check that ser+unser produces the same AddrMan by vasild
  8. DrahtBot cross-referenced this on May 24, 2021 from issue addrman: Make consistency checks a runtime option by jnewbery
  9. DrahtBot cross-referenced this on May 24, 2021 from issue addrman: improve performance by using more suitable containers by vasild
  10. 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.

  11. vasild approved
  12. vasild commented at 8:49 AM on May 24, 2021: contributor

    ACK 6627b0bab44a6e8392611182fdec4c4a85ba10a0

  13. hebasto force-pushed on May 24, 2021
  14. vasild approved
  15. vasild commented at 9:04 AM on May 24, 2021: contributor

    ACK 1b177a442a2e3749073cc189ddd404e74a902923

  16. jnewbery commented at 11:29 AM on May 24, 2021: member

    ACK 1b177a442a2e3749073cc189ddd404e74a902923

  17. move-only: Group and re-order CAddrMan members by access type
    Easy to verify with `git diff --color-moved=dimmed-zebra`.
    8caf60dbbe
  18. 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.

  19. hebasto force-pushed on May 24, 2021
  20. hebasto commented at 4:36 PM on May 24, 2021: member

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

  21. hebasto renamed this:
    refactor: Make CAddrMan protected members private ones
    refactor: Group and re-order CAddrMan members by access type
    on May 24, 2021
  22. jnewbery commented at 4:52 PM on May 24, 2021: member

    ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab

  23. practicalswift commented at 7:58 PM on May 24, 2021: contributor

    Concept ACK

  24. jarolrod commented at 4:32 AM on May 26, 2021: member

    ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab

    Patch looks correct

  25. vasild approved
  26. vasild commented at 1:32 PM on May 27, 2021: contributor

    ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab

  27. laanwj commented at 1:52 PM on May 27, 2021: member

    Code review ACK 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab

  28. laanwj merged this on May 27, 2021
  29. laanwj closed this on May 27, 2021
  30. hebasto deleted the branch on May 27, 2021
  31. sidhujag referenced this in commit f8c7c25a8d on May 27, 2021
  32. MarcoFalke referenced this in commit 3a2c84a6b5 on Jun 14, 2021
  33. gwillen referenced this in commit af3617ff29 on Jun 1, 2022
  34. bitcoin locked this on Aug 16, 2022
Contributors
hebastoDrahtBotvasildjnewberylaanwjpracticalswiftjarolrod
Labels
Refactoring
Linked (view graph)
#19238 refactor: Make CAddrMan::cs non-recursive
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC