Skip to content

rest/rpc: use more util::Join #32942

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

rest/rpc: use more util::Join #32942

wants to merge 3 commits into from
+37 −52

Conversation

stickies-v
Copy link
Contributor

@stickies-v stickies-v commented Jul 10, 2025 •
edited
Loading

There are a few places in the RPC and REST code where we manually join multiple items into a string. This PR removes a bunch of that logic and replaces it with util::Join. To minimize unnecessary container allocations and simplify implementations, the first commit slightly refactors util::Join so it can subsequently be used with range adaptors.

@stickies-v
util: make Join compatible with ranges
c2bb737 
By using a forwarding reference for `container`, the function can
be used with range adaptors that produce views that are not iterable
when const.
@stickies-v
rest: make rf_names a map to simplify code
31bbaab 
As a map, we can use views and util::Join on it to avoid duplicating
logic.
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 10, 2025 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32942.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK w0xlt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@stickies-v stickies-v mentioned this pull request Jul 10, 2025
Merged
@stickies-v
rpc: use more util::Join
e68a4ba 
Use Join where we manually concatenated before, or use ranges to
simplify the code.
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/runs/45741536315
LLM reason (✨ experimental): clang-tidy detected errors related to recursive call chains, causing the CI to fail.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

w0xlt
w0xlt reviewed Jul 10, 2025
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt 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 e68a4ba

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@w0xlt w0xlt w0xlt left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stickies-v @DrahtBot @w0xlt