Avoid unnecessary .expect()s for empty HeaderMap #768
Conversation
src/header/map.rs
Outdated
| Self::new_empty() | ||
| } | ||
| } | ||
|
|
||
| impl<T> HeaderMap<T> { | ||
| fn new_empty() -> Self { | ||
| HeaderMap { | ||
| mask: 0, | ||
| indices: Box::new([]), // as a ZST, this doesn't actually allocate anything | ||
| entries: Vec::new(), | ||
| extra_values: Vec::new(), | ||
| danger: Danger::Green, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| Self::new_empty() | |
| } | |
| } | |
| impl<T> HeaderMap<T> { | |
| fn new_empty() -> Self { | |
| HeaderMap { | |
| mask: 0, | |
| indices: Box::new([]), // as a ZST, this doesn't actually allocate anything | |
| entries: Vec::new(), | |
| extra_values: Vec::new(), | |
| danger: Danger::Green, | |
| } | |
| } | |
| HeaderMap { | |
| mask: 0, | |
| indices: Box::new([]), // as a ZST, this doesn't actually allocate anything | |
| entries: Vec::new(), | |
| extra_values: Vec::new(), | |
| danger: Danger::Green, | |
| } | |
| } | |
| } | |
| impl<T> HeaderMap<T> { |
I think it's simpler to make the new constructor initialize the empty map instead of adding a new method for it. Vec::new is implemented just like that, for example.
There was a problem hiding this comment.
That was my first inclination but it would mean we can't use Self::new() in the Default impl because new() is only defined for HeaderMap<HeaderValue>, not HeaderMap<T>. If we want to try to eliminate the extra method I'd suggest instead making new() and try_with_capacity(0) call Default::default() and putting the actual definition in there. Alternatively we can move the definition of new() to the more general impl block below but that might break calling code that is relying on new() to constrain the generic type.
There was a problem hiding this comment.
I went ahead and did this in the latest commit. After looking at https://doc.rust-lang.org/cargo/reference/semver.html#fn-generalize-compatible I wouldn't be opposed to just making new() more generic since that's considered a non-breaking change even if breaks type inference in consumers. I'm happy to do that here if that's what maintainers prefer.
Edit: here's what that would look like: akonradi-signal@7f118aa. The tests had to be updated to handle the type inference breakage.
|
Sent #769 to fix the nightly toolchain CI errors. They're unrelated to this PR. |
This change removes the Result::expect() calls in the constructors for an empty HeaderMap. These calls were provably not going to fail at runtime but rustc's inliner wasn't smart enough to figure that out: strings analysis of compiled binaries showed that the error message to the expect() still showed up in generated code. There are no behavioral differences as a result of this change.
akonradi-signal
force-pushed
the
empty-header-map-no-expect
branch
from
ce1bfa3 to
43228c1
Compare
This gives us one fewer named method since we can use default() in place of new_empty(). It preserves the property that `HeaderMap::new()` constrains the generic type to `T=HeaderValue`.
This change removes the Result::expect()/unwrap() calls in the constructors for an empty HeaderMap. These calls were provably not going to fail at runtime but rustc's inliner wasn't smart enough to figure that out: strings analysis of compiled binaries showed that the error message to the expect() still showed up in generated code.
There are no behavioral differences as a result of this change.