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 some unsafe magic for minimal optimizations, add .idea to .gitignore and fix some typos #71

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
/target
.idea/
34 changes: 17 additions & 17 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ pub struct ParseUrlError<Input> {
}

/// Defines the type of the host.
#[repr(u32)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum HostType {
Domain = 0,
Expand All @@ -75,16 +76,18 @@ pub enum HostType {

impl From<c_uint> for HostType {
fn from(value: c_uint) -> Self {
match value {
0 => Self::Domain,
1 => Self::IPV4,
2 => Self::IPV6,
_ => Self::Domain,
if value > 2 {
return Self::Domain;
}

// Safety: Prior to transmuting we checked if value is bigger than 2
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the mapping (number to enum) as a comment to here? Let's not also remove the knowledge we have since it might not be obvious at the first glance.

// and returned the default in that case.
unsafe { std::mem::transmute(value) }
Copy link
Member

Choose a reason for hiding this comment

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

It appears that it is extremely unsafe. Ref: https://x.com/tenellous/status/1819506534675894296

Is there any benchmark result you could share with us, that makes this change/risk worthy to land?

}
}

/// Defines the scheme type of the url.
#[repr(u32)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum SchemeType {
Http = 0,
Expand All @@ -98,16 +101,13 @@ pub enum SchemeType {

impl From<c_uint> for SchemeType {
fn from(value: c_uint) -> Self {
match value {
0 => Self::Http,
1 => Self::NotSpecial,
2 => Self::Https,
3 => Self::Ws,
4 => Self::Ftp,
5 => Self::Wss,
6 => Self::File,
_ => Self::NotSpecial,
if value > 6 {
return Self::NotSpecial;
}

// Safety: Prior to transmuting we checked if value is bigger than 6
// and returned the default in that case.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the mapping as a comment here as well?

unsafe { std::mem::transmute(value) }
}
}

Expand Down Expand Up @@ -228,7 +228,7 @@ impl Url {
}
}

/// Returns whether or not the URL can be parsed or not.
/// Returns whether the URL can be parsed or not.
///
/// For more information, read [WHATWG URL spec](https://url.spec.whatwg.org/#dom-url-canparse)
///
Expand Down Expand Up @@ -414,7 +414,7 @@ impl Url {
/// A fragment is the part of the URL with the # symbol.
/// The fragment is optional and, if present, contains a fragment identifier that identifies
/// a secondary resource, such as a section heading of a document.
/// In HTML, the fragment identifier is usually the id attribute of a an element that is
/// In HTML, the fragment identifier is usually the id attribute of an element that is
/// scrolled to on load. Browsers typically will not send the fragment portion of a URL to the
/// server.
///
Expand Down Expand Up @@ -628,7 +628,7 @@ impl Url {
unsafe { ffi::ada_has_credentials(self.0) }
}

/// Returns true if it has an host but it is the empty string.
/// Returns true if it has a host, but it is the empty string.
#[must_use]
pub fn has_empty_hostname(&self) -> bool {
unsafe { ffi::ada_has_empty_hostname(self.0) }
Expand Down
Loading