Conversation
@@ -522,7 +519,7 @@ impl<'a, 'b> Response<'a, 'b> { | |||
let mime = path | |||
.extension() | |||
.and_then(|ext| to_mime(&ext.to_string_lossy())) | |||
.unwrap_or(Mime(TopLevel::Application, SubLevel::Ext("octet-stream".into()), vec![])); | |||
.unwrap_or_else(|| Mime(TopLevel::Application, SubLevel::Ext("octet-stream".into()), vec![])); |
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.
I'm not sure about this, since the "octet-stream"
string will be converted and allocated even if the value isn't used. The question is which alternative is the cheapest.
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.
I’m unsure what you mean here. If the Mime(…)
value is to be unused, the closure won’t be called and the string won’t be allocated. And unwrap_or_else
is #[inline]
d, so I’d expect the closure itself to have no-cost.
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.
Oh, my! My brain must have had a meltdown. 😣 I thought, for some reason, that the change was from unwrap_or_else
to unwrap_or
. Ignore that comment 😅
Very nice! It's just that one case with The complicated types could possibly move out of the trait implementations, but I would rather just find a better alternative to that pattern. They can stay for now. The It's nice to see that the false positives are fewer this time 😄 |
Never mind, it's all good (except for my brain, apparently). Thanks for running these checks. @homu r+ |
📌 Commit e59e0d1 has been approved by |
⚡ Test exempted - status |
Fix some Clippy warnings There are 5 warnings left: - one `needless_lifetimes` which is a false positive (reported there: https://github.com/Manishearth/rust-clippy/issues/740#issuecomment-205826823); - 3 `type_complexity` warnings that I did not fix because they seem legit; - one `wrong_self_convention` which seems semi-legit too, although it’s a little surprising to have an `is_empty` function require a `&mut self`. Here they are, FYI: ```rust src/router/tree_router.rs:80:5: 110:6 warning: explicit lifetimes given in parameter types where they could be elided, #[warn(needless_lifetimes)] on by default src/router/tree_router.rs: 80 fn merge_router<'a, I: Iterator<Item = &'a [u8]> + Clone>(&mut self, state: InsertState<'a, I>, router: TreeRouter<T>) { src/router/tree_router.rs: 81 self.item.insert_router(state.clone(), router.item); src/router/tree_router.rs: 82 src/router/tree_router.rs: 83 for (key, router) in router.static_routes { src/router/tree_router.rs: 84 let next = match self.static_routes.entry(key.clone()) { src/router/tree_router.rs: 85 Occupied(entry) => entry.into_mut(), ... src/router/tree_router.rs:80:5: 110:6 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes src/router/mod.rs:357:21: 357:71 warning: very complex type used. Consider factoring parts into `type` definitions, #[warn(type_complexity)] on by default src/router/mod.rs:357 type Segments = RouteIter<Split<'a, u8, &'static fn(&u8) -> bool>>; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/router/mod.rs:357:21: 357:71 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#type_complexity src/router/mod.rs:365:21: 365:71 warning: very complex type used. Consider factoring parts into `type` definitions, #[warn(type_complexity)] on by default src/router/mod.rs:365 type Segments = RouteIter<Split<'a, u8, &'static fn(&u8) -> bool>>; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/router/mod.rs:365:21: 365:71 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#type_complexity src/router/mod.rs:393:21: 393:170 warning: very complex type used. Consider factoring parts into `type` definitions, #[warn(type_complexity)] on by default src/router/mod.rs:393 type Segments = FlatMap<<&'a I as IntoIterator>::IntoIter, <<T as Deref>::Target as Route<'a>>::Segments, fn(&'a T) -> <<T as Deref>::Target as Route<'a>>::Segments>; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/router/mod.rs:393:21: 393:170 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#type_complexity src/router/mod.rs:449:21: 449:34 warning: methods called `is_*` usually take self by reference or no self; consider choosing a less ambiguous name, #[warn(wrong_self_convention)] on by default src/router/mod.rs:449 pub fn is_empty(&mut self) -> bool { ^~~~~~~~~~~~~ src/router/mod.rs:449:21: 449:34 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#wrong_self_convention ```
There are 5 warnings left:
needless_lifetimes
which is a false positive (reported there: https://github.com/Manishearth/rust-clippy/issues/740#issuecomment-205826823);type_complexity
warnings that I did not fix because they seem legit;wrong_self_convention
which seems semi-legit too, although it’s a little surprising to have anis_empty
function require a&mut self
.Here they are, FYI: