Modernize util/strencodings and util/string: string_view and optional
#25001
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In addition, to make sure that no call site ignores the invalid decoding status, make the pf_invalid argument mandatory.
Base32/base64 are mechanisms for encoding binary data. That they'd decode to a string is just bizarre. The fact that they'd do that based on the type of input arguments even more so.
-BEGIN VERIFY SCRIPT- sed -i 's,ValidAsCString,ContainsNoNUL,g' $(git grep -l ValidAsCString) -END VERIFY SCRIPT-
|
Disclaimer: The description and most commits are stolen from #24764 (comment) |
|
Previous review: #24764 (review) re-ACK fa7078d only change is rebase and adding a scripted-diff 🍲 Show signatureSignature: |
|
Code review ACK fa7078d, found no issue |
|
Code review ACK fa7078d |
|
utACK fa7078d (as far as the commit that isn't mine goes) |
laanwj
added a commit
to bitcoin-core/gui
that referenced
this pull request
Apr 27, 2022
…ing: `string_view` and `optional` fa7078d scripted-diff: Rename ValidAsCString to ContainsNoNUL (MacroFake) e7d2fbd Use std::string_view throughout util strencodings/string (Pieter Wuille) 8ffbd14 Make DecodeBase{32,64} take string_view arguments (Pieter Wuille) 1a72d62 Generalize ConvertBits to permit transforming the input (Pieter Wuille) 78f3ac5 Make DecodeBase{32,64} return optional instead of taking bool* (Pieter Wuille) a65931e Make DecodeBase{32,64} always return vector, not string (Pieter Wuille) a4377a0 Reject incorrect base64 in HTTP auth (Pieter Wuille) d648b51 Make SanitizeString use string_view (Pieter Wuille) 963bc9b Make IsHexNumber use string_view (Pieter Wuille) 4006299 Make IsHex use string_view (Pieter Wuille) c1d165a Make ParseHex use string_view (Pieter Wuille) Pull request description: Make use of `std::string_view` and `std::optional` in the util/{strencodings, string} files. This avoids many temporary string/vector objects being created, while making the interface easier to read. Changes include: * Make all input arguments in functions in util/strencodings and util/string take `std::string_view` instead of `std::string`. * Add `RemovePrefixView` and `TrimStringView` which also *return* `std::string_view` objects (the corresponding `RemovePrefix` and `TrimString` keep returning an `std::string`, as that's needed in many call sites still). * Stop returning `std::string` from `DecodeBase32` and `DecodeBase64`, but return vectors. Base32/64 are fundamentally algorithms for encoding bytes as strings; returning `std::string` from those (especially doing it conditionally based on the input arguments/types) is just bizarre. * Stop taking a `bool* pf_invalid` output argument pointer in `DecodeBase32` and `DecodeBase64`; return an `std::optional` instead. * Make `DecodeBase32` and `DecodeBase64` more efficient by doing the conversion from characters to integer symbols on-the-fly rather than through a temporary vector. ACKs for top commit: MarcoFalke: re-ACK fa7078d only change is rebase and adding a scripted-diff 🍲 martinus: Code review ACK fa7078d, found no issue laanwj: Code review ACK fa7078d sipa: utACK fa7078d (as far as the commit that isn't mine goes) Tree-SHA512: 5cf02e541caef0bcd100466747664bdb828a68a05dae568cbcd0632a53dd3a4c4e85cd8c48ebbd168d4247d5c9666689c16005f1c8ad75b0f057d8683931f664
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 29, 2022
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Dec 21, 2022
…iterator bb5ea1d qt: Load PSBTs using istreambuf_iterator rather than istream_iterator (Andrew Chow) Pull request description: `istream_iterator` eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters. `istreambuf_iterator` is the correct thing to use here. This is a regression in 24.0. bitcoin/bitcoin#25001 accidentally changed the original `istreambuf_iterator` to `istream_iterator`. ACKs for top commit: furszy: Tested ACK bb5ea1d MarcoFalke: review ACK bb5ea1d 🍇 Tree-SHA512: 35d90eee3efdcb6a360af69ac1727f9f2837ea621297196de3136299f5de6d9975df4e425e1fc5b8813c1ddb2a4d60c3969e1d5d968953a4628ca45e37d3bf05
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 21, 2022
…er than istream_iterator bb5ea1d qt: Load PSBTs using istreambuf_iterator rather than istream_iterator (Andrew Chow) Pull request description: `istream_iterator` eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters. `istreambuf_iterator` is the correct thing to use here. This is a regression in 24.0. bitcoin#25001 accidentally changed the original `istreambuf_iterator` to `istream_iterator`. ACKs for top commit: furszy: Tested ACK bb5ea1d MarcoFalke: review ACK bb5ea1d 🍇 Tree-SHA512: 35d90eee3efdcb6a360af69ac1727f9f2837ea621297196de3136299f5de6d9975df4e425e1fc5b8813c1ddb2a4d60c3969e1d5d968953a4628ca45e37d3bf05
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make use of
std::string_viewandstd::optionalin the util/{strencodings, string} files.This avoids many temporary string/vector objects being created, while making the interface easier to read. Changes include:
std::string_viewinstead ofstd::string.RemovePrefixViewandTrimStringViewwhich also returnstd::string_viewobjects (the correspondingRemovePrefixandTrimStringkeep returning anstd::string, as that's needed in many call sites still).std::stringfromDecodeBase32andDecodeBase64, but return vectors. Base32/64 are fundamentally algorithms for encoding bytes as strings; returningstd::stringfrom those (especially doing it conditionally based on the input arguments/types) is just bizarre.bool* pf_invalidoutput argument pointer inDecodeBase32andDecodeBase64; return anstd::optionalinstead.DecodeBase32andDecodeBase64more efficient by doing the conversion from characters to integer symbols on-the-fly rather than through a temporary vector.