Merged
Conversation
Darksonn
reviewed
Jan 8, 2025
…_get_impl macro is implemented through buf_try_get_impl to avoid code duplication, and copy_to_slice is implemented through try_copy_to_slice.
Contributor
Author
|
@Darksonn @paolobarbolini I think I have made the necessary changes to the comments I have received so far. I have pushed the "Resove" button on each to indicate this (I hope this was the correct thing to do in this repo, it seems to vary). I hope you have the time next week to review my changes. Also, the CI needs to be started on the latest changes. |
Member
|
Thanks, clicking resolve is fine. 👍 |
Darksonn
reviewed
Jan 16, 2025
…d::io::Error using From. Used TryGetError as the parameter for panic_advance, and moved TryGetError to before panic_advance in lib.rs. Removed unwrap_advance, as calling panic_advance is now convenient enough.
Contributor
Author
|
@Darksonn I think all PR comments have been addressed now, although I was in doubt as to where it is best to define TryGetError (lib.rs or in buf_impl.rs). See also the answer to one of your comments above. |
Contributor
Author
This was referenced Jan 21, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Adds fallible
try_get_methods to Buf. The methods align with the existingget_methods. All of them, so this is a lot of lines but most of it is documentation. Note that I copied and adaptedbuf_get_implto maketry_buf_get_impl. It is possible to avoid some code duplication by callingbuf_get_implinstead, but that will checked the remaining bytes twice, andbuf_get_implis performance optimized so I chose not to do that.This code borrows some from the existing PR for this feature by @caelunshun, but that is old with lots of conflicts, so I chose to reimplement it from the current master instead.
I would rater have used from_be_bytes etc. for the floats as that seems more portable for NaN values, but it seems that the supported rust version is perhaps one minor version too old for that.
Closes #254