rpc: refactor: use string_view in Arg/MaybeArg #32983
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32983. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
ACK 319abd6
I like this refactoring — the general idea of reducing unnecessary string allocations via string_view makes sense.
I’m a bit unsure about the changes in mining.cpp and signmessage.cpp. It seems like we’re going from string → string_view → string, which introduces a bit of unnecessary churn and could lead to confusion down the line. I understand the intention may be to eventually update those functions to accept string_view(?), but without that broader change in place, it’s hard to see a clear benefit to doing it this way for now.
319abd6 to
b8772ef
Compare
|
Force-pushed from 319abd6 to b8772ef to remove a
In In |
b8772ef to
122b432
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
There was a problem hiding this comment.
re-ACK 122b432
Not blocking: I’d personally prefer keeping Arg<std::string> in signmessage.cpp if we're not planning to update MessageVerify to accept string_view instead of const std::string&. Is updating MessageVerify something you’d prefer to avoid at this stage?
Yes. It involves a cascade of changes, many touching consensus code, that require significant review and have mostly nothing to do with RPC, so it would blow up the scope of this PR. |
122b432 to
137319b
Compare
|
Force-pushed from 122b432 to 137319b to address silent merge conflict from #32845:
Otherwise no changes. |
Modernizes interface by not forcing users to deal with raw pointers, without adding copying overhead. Generalizes the logic of whether we return by value or by optional/pointer. In cases where functions take a `const std::string&` and it would be too much work to update them, a string copy is made (which was already happening anyway).
Update select functions that take a const std::string& to take a
std::string_view instead. In a next commit, this allows us to use
the {Arg,MaybeArg}<std::string_view> helper.
Use the {Arg,MaybeArg}<std::string_view> helper in all places where
it is a trivial change. In many places, this simplifies the logic
and reduces duplication of default values.
137319b to
24d4863
Compare
|
Rebased to address merge conflict from #32896, no other changes. |
The
RPCHelpMan::{Arg,MaybeArg}helpers avoid copying (potentially) large strings by returning them asconst std::string*(MaybeArg) orconst std::string&(Arg). ForMaybeArg, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g.EnsureUniqueWalletName) with raw pointers being implemented.This PR aims to improve on this by returning a trivially copyable
std::string_view(Arg) orstd::optional<std::string_view>(MaybeArg), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference usingstd::is_trivially_copyable_vinstead of defining the types manually.In cases where functions currently take a
const std::string&and it would be too much work / touching consensus logic to update them (signmessage.cpp), astd::stringcopy is made (which was already happening anyway).The last 2 commits increase usage of the
{Arg,MaybeArg}<std::string_view>helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.