Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Method::from_static #595

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WhyNotHugo
Copy link
Contributor

Allows creating constant Method instances, e.g.:

const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: #587

src/method.rs Outdated Show resolved Hide resolved
src/method.rs Outdated Show resolved Hide resolved
@WhyNotHugo WhyNotHugo force-pushed the const-methods branch 2 times, most recently from 5dc2a6b to d9a6f0b Compare March 27, 2023 20:05
src/method.rs Outdated Show resolved Hide resolved
@WhyNotHugo WhyNotHugo force-pushed the const-methods branch 2 times, most recently from b2fa38f to 7107737 Compare March 28, 2023 15:06
@WhyNotHugo
Copy link
Contributor Author

Oh, I just realised that CI failed due to panic! not being available on the MSRV. I'll try refactoring that.

@seanmonstar
Copy link
Member

We could probably update the MSRV, depending on what the version required is and how old it is.

@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented Apr 3, 2023

Rust 1.57.0, from 2021-12-02

@WhyNotHugo
Copy link
Contributor Author

For tokio, the current MSRV is 1.56.0.

@WhyNotHugo
Copy link
Contributor Author

I've updated the PR to panic without panic!. I used ([] as [u8; 0])[0];, which is already used as a const panic! hack in a couple of other places in the codebase.

We can remove this hack (both this new instance and existing ones) once the MSRV is bumped to 1.57.0 or greater.

@lperlaki
Copy link

I was wondering if we could add a variant StaticRef(&'static [u8]) to the Method Inner enum instead of panicking on longer than 15 byte methods?

@WhyNotHugo
Copy link
Contributor Author

I don't know of any methods that are longer than 15 bytes, so I'm not sure that it's worth the effort.

@2xsaiko
Copy link

2xsaiko commented Feb 20, 2024

What's this stuck on?

@lperlaki
Copy link

lperlaki commented Feb 21, 2024

I would also like to see this being merged as it helps with implementing a webdav server base on hyper

I don't know of any methods that are longer than 15 bytes, so I'm not sure that it's worth the effort.

recently I stubbled over the BASELINE-CONTROL webdav method wich is longe than 15 bytes (16)

I was wondering if we could add a variant StaticRef(&'static [u8]) to the Method Inner enum instead of panicking on longer than 15 byte methods?

figured out that this wouldn't work with the current implementation since the equality also checks the enum variant wich is required that Method can support pattern matching

@orium
Copy link

orium commented Nov 28, 2024

@seanmonstar Anything that needs to be done to land this? Happy to help if there's anything to do.

Allows creating constant `Method` instances, e.g.:

    const PROPFIND: Method = Method::from_static(b"PROPFIND");

Fixes: hyperium#587
@WhyNotHugo
Copy link
Contributor Author

I was wondering if we could add a variant StaticRef(&'static [u8]) to the Method Inner enum instead of panicking on longer than 15 byte methods?

I explored this a bit further.

Adding a new enum variant won't increase the total size of Inner. The associated data is a single reference, so it's also not bigger than the largest of the existing variants.

I've implemented your suggestions. This remove the conditional (compile-time) panic if len > 15.

@WhyNotHugo
Copy link
Contributor Author

At this point, waiting for feedback from the core devs.

b"PATCH" => Method::PATCH,
src => {
if src.len() <= 15 {
let inline = InlineExtension::from_static(src);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With an ExtensionStatic available, is there a benefit to keeping this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InlineExtension has no heap allocation and has the data available in the same allocation. Most HTTP methods fit in this case, and it has no overhead.

ExtensionStatic is a niche for unusually long methods, but has a heap allocation and an extra indirection.

We have three options here:

  • Keep both. No real downsides aside from another internal type, and does not increase the size of the Method type.
  • Keep only InlineExtension, restricting from_static to only methods of len <= 15.
  • Keep only ExtensionStatic, forcing any from_static method to use a heap allocation and an extra indirection.

Personally, I prefer the first choice the most, and the third one the least (since it introduces pointless overhead for most WebDAV methods, albeit minor).

@lperlaki
Copy link

I found the Problem with ExtensionStatic is that

const BASELINE_CONTROL: Method = Method::from_static(BASELINE-CONTROL);

let m = Method::from_bytes(BASELINE-CONTROL);

let is_baseline = match m {
    BASELINE_CONTROL => true,
    _ => false,
};

will not match.

@WhyNotHugo
Copy link
Contributor Author

I found the Problem with ExtensionStatic is that

const BASELINE_CONTROL: Method = Method::from_static(BASELINE-CONTROL);

let m = Method::from_bytes(BASELINE-CONTROL);

let is_baseline = match m {
    BASELINE_CONTROL => true,
    _ => false,
};

will not match.

I'll add a test for this case and figure out what's wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A method! macro to define methods at build time
5 participants