Conversation
49ceb1c to
54b211d
Compare
seanmonstar
left a comment
There was a problem hiding this comment.
Thanks for taking this on! 2 notes inline.
| ready!(conn | ||
| .as_mut() | ||
| .expect("client connection polled after completion") | ||
| .unwrap_or_else(|| unreachable!("client connection polled after completion")) |
There was a problem hiding this comment.
I think using expect in this case, polling a completed future, is normal. I wouldn't call this unreachable, because if the user does so, it sounds like we made a mistake.
But the docs of Future indicate the user should not do so. I don't think we need to document that behavior.
There was a problem hiding this comment.
Fixed. Thanks for the prompt review!
Please note, on rebase I hit more lints and have updated accordingly.
| /// | ||
| /// # Panics | ||
| /// | ||
| /// This future will panic if polled after it has already completed. |
There was a problem hiding this comment.
Same here, this is basically expected from the Future docs, we can leave it off.
d59e9ae to
30577fc
Compare
30577fc to
d5729db
Compare
d5729db to
e201317
Compare
|
I've addressed all findings and I've also gone and updated the commit message and formatting to meet repo guidelines. |
|
Alright, looks like we still need to fix the cargo fmt issue. |
This time it seems it was due to MSRV, I had't realized allow block reasons were recent. Sorry for the mistake. I've updated it now. |
e201317 to
cb8ce62
Compare
Towards #4071
Most of the panics are structurally unreachable, I attempted to reproduce them with adversarial workflows (panics, malformed inputs, etc) and I believe they are truly unreachable so I have marked them as
unreachable!(...). This will still panic if it is hit, but it makes the intent clear and avoids needing a panic doc.Two panics were reproducible and have had their panic docs written.
Connection::without_shutdown @ src/server/conn/http1.rs:184ReadBufCursor::advance @ src/rt/io.rs:346Both cases are pretty silly to ever hit, but documentation is free.