Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
Let's make sure they are at least all used in tests in some way (I haven't checked if this is the case). |
There was a problem hiding this comment.
Code review and tested ACK fae93c5
Tested by writing an Adhoc unit test klementtan@129b1e0 (feel free to copy the test into this PR)
Also, simplify unit tests with the CDataStream::str method.
Also, add Span<std::byte> interface to strencondings.
|
Thanks for the tests. However, I think the line is undefined behavior. The size of the span should be 6, so at most it is allowed to read [5]. I've pushed a commit with my own tests for all newly added functions. |
|
reACK faa3ec2. Verified that all the new @MarcoFalke Yup you're right, |
| char dummy; | ||
| while (len--) | ||
| s.read(&dummy, 1); | ||
| s.ignore(len); |
There was a problem hiding this comment.
This is the most tricky change to review I think; whether the behavior here stays the same in all cases, also in edge cases at the end of the stream.
There was a problem hiding this comment.
The only change should be in the error message and a missing call to memcpy.
-CDataStream::read(): end of data
+CDataStream::ignore(): end of dataThe commit is unrelated and not needed for the changes here, so I am happy to remove. If it will be removed, I'd later on need to replace char dummy; with std::byte dummy;.
There was a problem hiding this comment.
I'm fine with keeping it. I personally think the change is correct, just mentioned it as a point of focus for reviewers.
|
Code review ACK faa3ec2 |
faa3ec2 span: Add std::byte helpers (MarcoFalke) fa18038 refactor: Use ignore helper when unserializing an invalid pubkey (MarcoFalke) fabe18d Use value_type in CDataStream where possible (MarcoFalke) Pull request description: This adds (currently unused) span std::byte helpers, so that they can be used in new code. The refactors are also required for bitcoin#23438, but they are split up because the other pull doesn't compile with msvc right now. The third commit is not needed for the other pull, but still nice. ACKs for top commit: klementtan: reACK faa3ec2. Verified that all the new `std::byte` helper functions are tested. laanwj: Code review ACK faa3ec2 Tree-SHA512: b1f6af39f03ea4dfebf20d4a8538fa993a6104e7fc92ddf0c4606a7efc3ca9a8c1a4741d98a1418569c11bb9ce9258bf0c0c06d93d85ed7e208902a2db04e407
faa3ec2 span: Add std::byte helpers (MarcoFalke) fa18038 refactor: Use ignore helper when unserializing an invalid pubkey (MarcoFalke) fabe18d Use value_type in CDataStream where possible (MarcoFalke) Pull request description: This adds (currently unused) span std::byte helpers, so that they can be used in new code. The refactors are also required for bitcoin#23438, but they are split up because the other pull doesn't compile with msvc right now. The third commit is not needed for the other pull, but still nice. ACKs for top commit: klementtan: reACK faa3ec2. Verified that all the new `std::byte` helper functions are tested. laanwj: Code review ACK faa3ec2 Tree-SHA512: b1f6af39f03ea4dfebf20d4a8538fa993a6104e7fc92ddf0c4606a7efc3ca9a8c1a4741d98a1418569c11bb9ce9258bf0c0c06d93d85ed7e208902a2db04e407
5 participants
This adds (currently unused) span std::byte helpers, so that they can be used in new code.
The refactors are also required for #23438, but they are split up because the other pull doesn't compile with msvc right now.
The third commit is not needed for the other pull, but still nice.