-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify Read trait blocking behavior #625
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
9b1e754
Clarify Read trait blocking behavior
ivmarkov 45fcad0
Update embedded-io/src/lib.rs
ivmarkov 92dbaf2
Re-add an unintentionally removed paragraph
ivmarkov f3558ac
Add identical changes to the async Read variant
ivmarkov d745418
Address review feedback
ivmarkov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit: this line looks like it applies only to the "if there are bytes available to read" case, while it should apply to both. Perhaps split it to a new paragraph and remove the
-
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, but it is applicable only to the "if there are bytes available to read" case, as it talks about - if there are bytes available to read in the first place - how many of those would be returned - i.e. not necessarily all of them (because (a) the buffer might actually be shorter than what is available (b) because the implementation just decides so, for whatever other reasons).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies to the first case as well, doesn't it? In "Once at least one (or more) bytes become available", one could also ask if it's guaranteed that all available bytes are returned. The answer is "it is not guaranteed", but it's not completely obvious from the description as it is written now.
But as Dirbaio wrote, it is a "minor nit". Personally, I would not expect to have such a guarantee when reading the text you proposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how come the first case is unclear that only a subset of the just-read bytes are returned, given that it says (emphasis mine):
"
buf
, and the amount is returned..."
"a non-zero amount of those". Do we really need to be more explicit than that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah be it. If two folks find it unclear then it is probably unclear. :(
I'll re-arrange so that the problematic text is a separate paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jannic I've re-arranged it now so that the problematic text is a separate paragraph. If you can take a quick look - thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO the only possible source of confusion was the asymmetry between the two cases. Yes, in general I agree that the wording "a non-zero amount of those" was sufficiently clear. But then the second case, in addition to that, explicitly stated that it's allowed to return less than available. For the first case, that sentence was missing. Readers may wonder why? Is there a difference in guarantees?
Again, this was a very minor detail. But if we can avoid this possible confusion by just putting the last sentence in a separate paragraph, as you now did, we should do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, don't want to over-complicate it, but now - with the problematic text being a separate paragraph - the next paragraph that starts with "This waiting behavior is important for the cases where
Read
..." becomes unclear... in that readers might be confused that it only refers to the newly-born "Note..." paragraph, while it refers to everything mentioned above.Any brilliant ideas? Or is it just my mind that finds the new arrangement also problematic?