Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jul 19, 2022

Doing anything else will just lead to more verbose and inconsistent code.

Copy link
Contributor

@jamesob jamesob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, will review shortly

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa32b1b - all instances of Assert(m_node.chainman) in node/interfaces replaced with chainman(), which is the same thing.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK fa32b1b

  • I agree with making the code less verbose while maintaining the same logic behind it.
  • Verified that using chainman() is equivalent to Assert(m_node.chainman)
ChainstateManager& chainman() { return *Assert(m_context->chainman); }
  • Verified that all instances of Assert(m_node.chainman) are replaced with chainman().

@jonatack
Copy link
Member

Post-merge ACK

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 20, 2022
…ChainImpl

fa32b1b refactor: Use chainman() helper consistently in ChainImpl (MacroFake)

Pull request description:

  Doing anything else will just lead to more verbose and inconsistent code.

ACKs for top commit:
  fanquake:
    ACK fa32b1b - all instances of `Assert(m_node.chainman)` in node/interfaces replaced with `chainman()`, which is the same thing.
  shaavan:
    Code Review ACK fa32b1b

Tree-SHA512: a417680f79c150e4431aa89bc9db79fdf2dd409419081eb243194837b4ab8d16434165393f39a157473802753843e8c5314ad05c569b4e9221ce29a9fd1cefb8
@bitcoin bitcoin locked and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants