Modernize util/strencodings and util/string: `string_view` and `optional` by sipa · Pull Request #24764 · bitcoin/bitcoin

sipa · 2022-04-04T19:14:59Z

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.

DrahtBot · 2022-04-04T23:40:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24742 ([POC] build: prune Boost headers in depends by fanquake)
#24185 (refactor: only use explicit reinterpret/const casts, not implicit by PastaPastaPasta)
#24058 (BIP-322 basic support by kallewoof)
#23595 (util: Add ParseHexstd::byte() helper by MarcoFalke)
#22953 (refactor: introduce single-separator split helper (boost::split replacement) by theStack)
#22087 (Validate port-options by amadeuszpawlik)

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.

laanwj · 2022-04-05T08:03:17Z

Concept ACK

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).

Returning a string_view can be somewhat dangerous (in combination with auto? also when the original string goes out of scope, especially if it's a temporary), so giving these functions new names is a good idea.

src/util/strencodings.cpp

maflcko

Returning a string_view can be somewhat dangerous (in combination with auto? also when the original string goes out of scope, especially if it's a temporary), so giving these functions new names is a good idea.

A new name is good, but doesn't solve the problem. I think this can be fixed with an attribute: #20493

I think this can be merged, but I left a few nits and questions.

review ACK 6e1f14780452d956503f1dbc3c91b17b44287f77 💢

Show signature

Signature:

src/util/strencodings.cpp

src/httprpc.cpp

src/test/base32_tests.cpp

src/test/base64_tests.cpp

src/i2p.cpp

src/test/base32_tests.cpp

src/test/base64_tests.cpp

src/util/strencodings.cpp

laanwj · 2022-04-05T11:04:15Z

A new name is good, but doesn't solve the problem. I think this can be fixed with an attribute: #20493

Agree, I just meant that putting a string_view-returning function as a drop-in replacement would have been a very bad idea, a new function with a new name means that people using it can reason about the risks anew. Sure, if an attribute can help avoid such problems more widely that's of course even better.

maflcko · 2022-04-05T13:57:08Z

review ACK e4092c5 🐤

Only changes:

Remove confusing (signed char) cast
Use shorter str.substr(0, 2) == "0x" over manual matching/implementation
A minor change in a unit test
Whitespace changes

Show signature

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK e4092c5230bdd1f6439c70fa22b68381e5cc6899 🐤

Only changes:
* Remove confusing (signed char) cast
* Use shorter str.substr(0, 2) == "0x" over manual matching/implementation
* A minor change in a unit test
* Whitespace changes
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjhQQwAsEPZBkw/LyKgYc/xITU1DL5Oausc0jYNBH7WrBEh055OyfUZ5NgxM87K
bcJTNQMCmLVfWnOcg0FUELaXCGbqua8qPgiKwQT5nxlHN0DXpakSSWrkGh2HKIGe
zwPKwNrMEH7TrCs3Tkn3ZTkiHI3gl+AFTL5zdw1b/sMwPapeZLDqoAQZXTqj+LRB
0d44jHX6oO5faHRI6p+fErVtg40Q+3s49CkYXbTl/u1HEWaKcWBxk0wZ9JHUUWuY
Z2K1OWDWH/KuR/gYlUUuVfJk5jgG6NqNgzErLUjCtYjQ6LGpI90e7ud/ahv2Xndb
vRzi/r/XAQrzlYLKM4ZuoeiTTSKxLJHD6353KVgPVKETioM6o1eyn3qWpg5JwnT4
VglcsozZzoVbSdghoQ397NhFjI+Lfs+d60EcUB8lzPgzP00gAo2smlXXM0QMBRV/
XOweIVcmYulLYK0LnBXbQy4GSqyE06JsIKpugwFJEcKb7mkdJSdJ8Xg4xxwwYyjR
CdV7knXn
=yyjx
-----END PGP SIGNATURE-----

Empact · 2022-04-06T18:17:25Z

Concept ACK - ~~the ConvertBits functionality is unused in this PR - maybe it could be saved for a time when it is needed?~~ (misread)

nits:

Mild preference for more strict separation between refactoring and behavior modification - would make this easier to ACK, at least in part
There is a check in IsHex (% 2 == 0) that could be applied to IsHexNumber
Could inline or return early in some cases where you're converting to return optional (previous implementation was constrained by the need to set the val) 889a5f1

sipa · 2022-04-06T18:21:02Z

Mild preference for more strict separation between refactoring and behavior modification - would make this easier to ACK, at least in part

Yeah, I'll separate them (now that we figured out the issue with ignoring the pfInvalid flag in the http auth logic).

There is a check in IsHex (% 2 == 0) that could be applied to IsHexNumber

That would be incorrect. Hex numbers don't need to be a multiple of 2 characters (0xF is a valid hex number, e.g.).

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.

maflcko

re-ACK 5ea072e only effective change is adding a "return false" 🌷

Show signature

Signature:

martinus · 2022-04-19T06:22:05Z

src/util/string.h

 * Check if a string does not contain any embedded NUL (\0) characters
 */
-[[nodiscard]] inline bool ValidAsCString(const std::string& str) noexcept
+[[nodiscard]] inline bool ValidAsCString(std::string_view str) noexcept


I think the name ValidAsCString is now a bit dangerous. I'd assume that it returns true when the argument is 0 terminated at the end (which was the case for the const std::string& str argument), but with a std::string_view argument this might not be the case.

So now even when ValidAsCString(str) returns true, it is not allowed to use e.g. strlen(str.data()).

I would rename the method to something like ContainsNoNUL to make it more clear what's checked, or keep the const std::string& argument for now

According to the documentation, it only checks if a string has embedded \0 characters. That std::string is zero-terminated internally is an implementation detail, the caller is still responsible for passing c_str() (and never .data()) when passing it to a function that takes a C string.

it is not allowed to use e.g. strlen(str.data()).

I'd really frown on code like that in any case.

Edit: The only time it'd be remotely acceptable to pass .data() as a C string would be if there is an embedded \0 in the array, which is exactly what this function avoids 😄

well, maybe it's just me, but from the interface ValidAsCString(std::string_view str) I'd guess that it returns true when it actually finds a \0 somewhere, but it's exactly the opposite 🙂

I have no objection to adding a scripted-diff to rename ValidAsCString to ContainsNoNUL. Either in this pull or as a follow-up.

@martinus See #25001

DrahtBot · 2022-04-26T10:06:51Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

maflcko · 2022-04-27T12:07:07Z

I am happy to pick this up if you are no longer working on it

sipa · 2022-04-27T12:08:56Z

@MarcoFalke Go for it.

sipa mentioned this pull request Apr 4, 2022

refactor: Remove ParseHex(const char*) from public interface #24751

Closed

sipa force-pushed the 202204_stringview branch from a1e5d7d to 6e1f147 Compare April 4, 2022 19:48

DrahtBot added GUI P2P RPC/REST/ZMQ Utils/log/libs labels Apr 4, 2022

maflcko added Refactoring and removed GUI RPC/REST/ZMQ P2P Utils/log/libs labels Apr 5, 2022

laanwj reviewed Apr 5, 2022

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

maflcko approved these changes Apr 5, 2022

View reviewed changes

sipa force-pushed the 202204_stringview branch from 6e1f147 to e4092c5 Compare April 5, 2022 13:45

This was referenced Apr 5, 2022

BIP-322 basic support #24058

Closed

util: Add ParseHex<std::byte>() helper #23595

Merged

refactor: introduce single-separator split helper (boost::split replacement) #22953

Merged

Validate port-options #22087

Merged

DrahtBot mentioned this pull request Apr 8, 2022

refactor: only use explicit reinterpret/const casts, not implicit #24185

Closed

sipa added 5 commits April 8, 2022 23:34

Make ParseHex use string_view

f80dedc

Make IsHex use string_view

6208dc4

Make IsHexNumber use string_view

d44fc2c

Make SanitizeString use string_view

2f15277

Reject incorrect base64 in HTTP auth

196c5ac

In addition, to make sure that no call site ignores the invalid decoding status, make the pf_invalid argument mandatory.

sipa added 5 commits April 8, 2022 23:34

Make DecodeBase{32,64} always return vector, not string

e779a46

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.

Make DecodeBase{32,64} return optional instead of taking bool*

5a103b7

Generalize ConvertBits to permit transforming the input

1d980ff

Make DecodeBase{32,64} take string_view arguments

e19891c

Use std::string_view throughout util strencodings/string

5ea072e

sipa force-pushed the 202204_stringview branch from e4092c5 to 5ea072e Compare April 9, 2022 03:35

This was referenced Apr 10, 2022

build: prune Boost headers in depends #24742

Closed

refactor: Split ArgsManager out of util/system #24455

Closed

maflcko approved these changes Apr 11, 2022

View reviewed changes

martinus reviewed Apr 19, 2022

View reviewed changes

DrahtBot added the Needs rebase label Apr 26, 2022

fanquake mentioned this pull request Apr 26, 2022

util: Pass size to ParseHex to assist preallocation #18061

Closed

sipa closed this Apr 27, 2022

maflcko mentioned this pull request Apr 27, 2022

Modernize util/strencodings and util/string: string_view and optional #25001

Closed

bitcoin locked and limited conversation to collaborators Apr 27, 2023

Conversation

sipa commented Apr 4, 2022

Uh oh!

DrahtBot commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Apr 5, 2022

Uh oh!

maflcko commented Apr 5, 2022

Uh oh!

Empact commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 6, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

martinus Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

laanwj Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinus Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 26, 2022

Uh oh!

maflcko commented Apr 27, 2022

Uh oh!

sipa commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Apr 4, 2022 •

edited

Loading

laanwj commented Apr 5, 2022 •

edited

Loading

Empact commented Apr 6, 2022 •

edited

Loading

laanwj Apr 21, 2022 •

edited

Loading