Skip to content

refactor: Pass reference to LookUpStats #25222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Sep 13, 2022
Merged

refactor: Pass reference to LookUpStats #25222

maflcko merged 1 commit into from
Sep 13, 2022

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 26, 2022

I find it confusing to have an interface that accepts nullptr, but immediately crashes the program when someone does pass nullptr.

Fix that.

Also some include fixups.

@jonatack
Copy link
Member

jonatack commented May 26, 2022

Concept ACK

CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
src/rpc/blockchain.cpp:827:            CBlockIndex& block_index = *Assert(WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(view->GetBestBlock())));

^---- failure generated from lint-assertions.py

@maflcko
Copy link
Member Author

maflcko commented May 27, 2022

Thx, fixed

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 2, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

This was referenced Jun 2, 2022
Draft
Closed
@DrahtBot DrahtBot mentioned this pull request Jun 29, 2022
Merged
@jonatack
Copy link
Member

jonatack commented Jul 20, 2022

Approach ACK, changes LGTM on first read over GitHub, checked that a lifetimebound annotation isn't needed.

aureleoules
aureleoules reviewed Sep 13, 2022
View reviewed changes
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

ACK faa3d38

@maflcko maflcko merged commit 141540a into bitcoin:master Sep 13, 2022
@maflcko maflcko deleted the 2205-ref-stat-😶 branch September 13, 2022 12:25
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 13, 2022
Merge bitcoin#25222: refactor: Pass reference to LookUpStats
68233ea 
faa3d38 refactor: Pass reference to LookUpStats (MacroFake)

Pull request description:

  I find it confusing to have an interface that accepts nullptr, but immediately crashes the program when someone does pass nullptr.

  Fix that.

  Also some include fixups.

ACKs for top commit:
  aureleoules:
    ACK faa3d38

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

Reviewers

@dergoegge dergoegge Awaiting requested review from dergoegge

@mzumsande mzumsande Awaiting requested review from mzumsande

1 more reviewer

@aureleoules aureleoules aureleoules left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maflcko @jonatack @DrahtBot @aureleoules