Fix wrong comments and add corresponding tests#78
Conversation
|
Thanks for the patch! Please fix the CI and I'll be happy to approve your changes :-). |
| /// # Panics | ||
| /// | ||
| /// Panics if the capacity is `0`, or larger than `usize::MAX / 2`. | ||
| /// Panics if the capacity is `0`, or overflows. |
There was a problem hiding this comment.
This panic message was copied from https://docs.rs/tokio/latest/tokio/sync/broadcast/fn.channel.html, and I think we should not change.
There was a problem hiding this comment.
This panic message was copied from https://docs.rs/tokio/latest/tokio/sync/broadcast/fn.channel.html, and I think we should not change.
But it actually is wrong. As capacity <= usize::MAX / 2, it will still panic(capacity overflows) as I shown in https://github.com/jplatte/eyeball/issues/77. So I think changing it to "Panics if the capacity is 0, or overflows." is more accurate.
There was a problem hiding this comment.
Okay, the code is here, https://github.com/tokio-rs/tokio/blob/17d8c2b29d94550f504d8fd76d8d8aaf66095864/tokio/src/sync/broadcast.rs#L543-L546, so it's not larger than, but larger or equal to.
There was a problem hiding this comment.
I'm sure tokio would be interesting in a doc fix too.
There was a problem hiding this comment.
Actually if capacity is a large enough number that is less than usize::MAX/2, it will still panic because of capacity overflow, so I think this is an obvious error. The comment should be changed to "Panics if the capacity is 0, or overflows. " in my view.
There was a problem hiding this comment.
Okay, the code is here, https://github.com/tokio-rs/tokio/blob/17d8c2b29d94550f504d8fd76d8d8aaf66095864/tokio/src/sync/broadcast.rs#L543-L546, so it's not larger than, but larger or equal to.
I noticed that contributor of tokio has confirmed my issue and make PR in tokio-rs/tokio#7352, could you please merge my PR?
jplatte
left a comment
jplatte
left a comment
There was a problem hiding this comment.
The doc updates are good, but I have some other comments.
| #[allow(missing_docs)] | ||
| pub fn new(inner: &'o mut ObservableVector<T>) -> Self { |
There was a problem hiding this comment.
Could you please move this file to tests/it as panic.rs or something like that? You'll also have to add a mod declaration for it in it/main.rs.
There was a problem hiding this comment.
Could you please move this file to
tests/itaspanic.rsor something like that? You'll also have to add amoddeclaration for it init/main.rs.
OK.
| let mut inner = ObservableVector::default(); | ||
| let mut txn = ObservableVectorTransaction::new(&mut inner); |
There was a problem hiding this comment.
| let mut inner = ObservableVector::default(); | |
| let mut txn = ObservableVectorTransaction::new(&mut inner); | |
| let mut ob = ObservableVector::default(); | |
| let mut txn = ob.transaction(); |
3 participants
Fix #77 .