Make Headers::remove return the header

[untitaker]

Fix #891

[untitaker]

Because a H may be unsized, I probably can only return Option<Box<Header + Send + Sync>> here. However, then I'm unsure how I'd be casting the box to a concrete type.

[untitaker]

To be clear, I get a type conflict exactly at this spot: Expected H, got Box<Header + Send + Sync>. The first idea here is to add a new downcast_* method to impl Header + Send + Sync (again analogous to the get methods), but I don't know how to implement such a method.

[seanmonstar]

Ah yes, of course, because the typed is storing them as Box<Header>. I think there is a way to add a downcast taking self by value, such as something like fn downcast(self: Box<Self>), but I'll toy on playground to find the right thing and report back.

[seanmonstar]

Adding this to the impl Header + Send + Sync should do it:

#[inline]
unsafe fn downcast_unchecked<T: 'static>(self: Box<Self>) -> T {
    *(mem::transmute::<*mut _, (*mut (), *mut ())>(Box::into_raw(self)).0 as *mut T)
}

Then you can call val.downcast_unchecked() after having gotten the type from the TypeId.

[untitaker]

Unfortunately this doesn't work, apparently I can't deref *mut T.

[seanmonstar]

Oh right. We can convert the *mut T into a Box<T> for free, with Box::from_raw, and then we can deref move out of the box:

*Box::from_raw(mem::transmute::<*mut _, (*mut (), *mut ())>(Box::into_raw(self)).0 as *mut T)

[untitaker]

Amazing.

[seanmonstar]

Oh woops, I didn't realize that the internal Item would need a new method, making this less easy than I suspected. Still, we should be able to figure it out.

[untitaker]

This is actually incorrect. None may also be returned if the removed header was not parseable.

I wonder if we should return Option<(Raw, Option<H>)> or something like that instead, to enable as many usecases as possible.

[seanmonstar]

Hm, I don't love that the return value could be more complicated, but how about this crazy idea: Option<Result<H, Raw>>...

I'd personally just drop it, but is there a convincing case where the raw is still needed to justify complicating the return type?

[untitaker]

I can't think of a reason why Raw is needed, but with the current return type there is no upgrade path for people who were checking the boolean value, as .is_some() is false even if the header was removed, but could not be parsed.

[untitaker]

Therefore Option<Option<H>> or Option<Result<H, ()>> would be appropriate if we don't want to bother with Raw.

[seanmonstar]

Both feel yucky. I'd still prefer to just have Option<H>. The upgrade path could be to check headers.has() before calling remove().

[untitaker]

Fair enough, I will update the docstring then.

[untitaker]

@seanmonstar This should be ready to merge.

[seanmonstar]

Looks right to me. Seems travis-ci's OSX containers are lagging, all the linux tests pass, so I'll merge anyways.

[untitaker]

Thanks!