refactor: Drop unsafe AsBytePtr function #27978
Merged
Conversation
This file contains hidden or 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
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
sipa
reviewed
Jun 26, 2023
Contributor
Author
|
Updated 3fca8c3 -> 2109a14 ( |
Member
|
Needs rebase lgtm ACK 2109a14 🐭 Show signatureSignature: |
This was referenced Jun 27, 2023
Contributor
Author
|
Rebased 2109a14 -> 650ca0d ( |
jonatack
reviewed
Jun 27, 2023
src/wallet/bdb.h
Outdated
| { | ||
| return {reinterpret_cast<const std::byte*>(dbt.get_data()), dbt.get_size()}; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Would it make sense for SpanFromDbt() to be a static method in src/wallet/bdb.cpp instead, which contains its only callers?
Replace calls to AsBytePtr with direct calls to AsBytes or reinterpret_cast. AsBytePtr is just a wrapper around reinterpret_cast. It accepts any type of pointer as an argument and uses reinterpret_cast to cast the argument to a std::byte pointer. Despite taking any type of pointer as an argument, it is not useful to call AsBytePtr on most types of pointers, because byte representations of most types will be implmentation-specific. Also, because it is named similarly to the AsBytes function, AsBytePtr looks safer than it actually is. Both AsBytes and AsBytePtr call reinterpret_cast internally and may be unsafe to use with certain types, but AsBytes at least has some type checking and can only be called on Span objects, while AsBytePtr can be called on any pointer argument. Co-authored-by: Pieter Wuille <[email protected]>
Contributor
Author
|
Updated 650ca0d -> 7c85361 ( |
Member
|
re-ACK 7c85361 The OP and commit message might need to be updated a bit. |
Member
Member
|
ACK 7c85361 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 30, 2023
7c85361 refactor: Drop unsafe AsBytePtr function (Ryan Ofsky) Pull request description: Replace calls to `AsBytePtr` with calls to `AsBytes` or `reinterpret_cast`. `AsBytePtr` is just a wrapper around `reinterpret_cast`. It accepts any type of pointer as an argument and uses `reinterpret_cast` to cast the argument to a `std::byte` pointer. Despite taking any type of pointer as an argument, it is not useful to call `AsBytePtr` on most types of pointers, because byte representations of most types will be platform specific or undefined. Also, because it is named similarly to the `AsBytes` function, `AsBytePtr` looks safer than it actually is. Both `AsBytes` and `AsBytePtr` call reinterpret_cast internally and may be unsafe to use with certain types, but AsBytes at least has some type checking and can only be called on `Span` objects, while `AsBytePtr` can be called on any pointer argument. The change was motivated by discussion on bitcoin#27973 and bitcoin#27927 and is compatible with those PRs ACKs for top commit: jonatack: re-ACK 7c85361 sipa: utACK 7c85361 achow101: ACK 7c85361 Tree-SHA512: 200d858b1d4d579f081a7f9a14d488a99713b4918b4564ac3dd5c18578d927dbd6426e62e02f49f04a3fa73ca02ff7109c495cb0b92bec43c27d9b74e2f95757
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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.
Replace calls to
AsBytePtrwith calls toAsBytesorreinterpret_cast.AsBytePtris just a wrapper aroundreinterpret_cast. It accepts any type of pointer as an argument and usesreinterpret_castto cast the argument to astd::bytepointer.Despite taking any type of pointer as an argument, it is not useful to call
AsBytePtron most types of pointers, because byte representations of most types will be platform specific or undefined. Also, because it is named similarly to theAsBytesfunction,AsBytePtrlooks safer than it actually is. BothAsBytesandAsBytePtrcall reinterpret_cast internally and may be unsafe to use with certain types, but AsBytes at least has some type checking and can only be called onSpanobjects, whileAsBytePtrcan be called on any pointer argument.The change was motivated by discussion on #27973 and #27927 and is compatible with those PRs