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

Make port: None equal to port: default_port #133

Merged
merged 3 commits into from Nov 10, 2015

Conversation

untitaker
Copy link
Contributor

Fix #132

Review on Reviewable

@untitaker
Copy link
Contributor Author

cc @SimonSapin

@untitaker
Copy link
Contributor Author

Will add tests if you agree with the concept.

@@ -226,7 +226,7 @@ pub enum SchemeData {
}

/// Components for URLs in a *relative* scheme such as HTTP.
#[derive(PartialEq, Eq, Clone, Debug, Hash, PartialOrd, Ord)]
#[derive(Clone, Debug, Hash, PartialOrd, Ord)]

Choose a reason for hiding this comment

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

If you're not deriving PartialEq, you can't derive Hash, PartialOrd, or Ord.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should rustc catch that?

Copy link
Member

Choose a reason for hiding this comment

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

It should, yes.

Hash is fine, it's the Ord traits which are problematic.

Choose a reason for hiding this comment

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

Probably... please file a bug (https://github.com/rust-lang/rust/issues/new).

Choose a reason for hiding this comment

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

Err, how is Hash not a problem. From https://doc.rust-lang.org/nightly/std/hash/trait.Hash.html: "if two keys are equal, their hashes should also be equal".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can rustc derive a correct Eq impl from a custom Hash impl?

Copy link
Member

Choose a reason for hiding this comment

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

Nope. It doesn't, and it's not possible to either, since there isn't enough information on what choices to make. In general an unsolvable problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? Return hash(self) == hash(other) from eq method.

Anyway, thanks for the quick review, I'll get to work.

On Mon, Nov 02, 2015 at 01:15:37PM -0800, Manish Goregaokar wrote:

@@ -226,7 +226,7 @@ pub enum SchemeData {
}

/// Components for URLs in a relative scheme such as HTTP.
-#[derive(PartialEq, Eq, Clone, Debug, Hash, PartialOrd, Ord)]
+#[derive(Clone, Debug, Hash, PartialOrd, Ord)]

Nope. It doesn't, and it's not possible to either, since there isn't enough information on what choices to make. In general an unsolvable problem.


Reply to this email directly or view it on GitHub:
https://github.com/servo/rust-url/pull/133/files#r43683364

Copy link
Member

Choose a reason for hiding this comment

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

Hashes may have collisions. Equal keys imply equal hashes, not the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW I've filed the derive(Hash) issue against rustc: rust-lang/rust#29758

@untitaker
Copy link
Contributor Author

I've updated the PR with custom Hash and Eq impls.

Should I do the same for Ord? Do we even need to order URLs?

@untitaker
Copy link
Contributor Author

Any update on this? Is there something left to do?

@Manishearth
Copy link
Member

LGTM, but I'll wait for Simon to review it

@SimonSapin
Copy link
Member

Yes, Ord and PartialOrd should be consistent with Eq and PartialEq. Please change them to use the identity key as well, and add those tests you mentioned :)

I also think that ordering URLs doesn't make sense, but Cargo wants to sort things that contain them: #114

@untitaker
Copy link
Contributor Author

  • Added some tests for equality.
  • I'm not sure how ordering should look like. Do we want the same exact ordering behavior as before, i.e. do we need extensive tests? Or is breakage fine as long as it doesn't happen too often? It's "obvious" that this new impl is deterministic. cc @alexcrichton
  • The hashing tests are ugly, check_eq was supposed to be a macro but then I somehow hit a ICE

}


impl PartialEq for RelativeSchemeData {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be interested into factoring out all these boilerplate impls into a new crate. Would that be something useful, and would it be worth adding as dependency?

I haven't decided on an API yet, but perhaps a macro that impls PartialEq, Hash, etc given a type and a function like get_identity_key.

@SimonSapin
Copy link
Member

@bors-servo r+

Looks good, thanks!

It’s not important that the ordering is the same as before. Only that it’s consistent (with the usual properties: total, anti-symmetric, transitive) within one execution of a program. The current impls are fine. I don’t think ordering tests are important here, the impls are very straightforward.

Yes, the impls are boilerplate-ish, but they’re only one line each. I’m skeptical that it’s useful to have a crate just for this, at least until there’s more than one thing that would use it.

@bors-servo
Copy link
Contributor

📌 Commit ab7f6fc has been approved by SimonSapin

@bors-servo
Copy link
Contributor

⌛ Testing commit ab7f6fc with merge fedf144...

bors-servo pushed a commit that referenced this pull request Nov 10, 2015
Make port: None equal to port: default_port

Fix #132

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/133)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - travis

@bors-servo bors-servo merged commit ab7f6fc into servo:master Nov 10, 2015
@untitaker
Copy link
Contributor Author

within one execution of a program

I don't exactly know where Cargo uses them, but I thought that it'd produce massive diffs for e.g. .lock-files when upgrading cargo and then running cargo update. I guess that's not a real issue though.

@SimonSapin
Copy link
Member

@alexcrichton, how do you feel about the ordering of URLs changing?

@alexcrichton
Copy link
Contributor

Does this radically change how URLs are ordered, or is it basically in just some niche cases the ordering gets "more correct" ?

@SimonSapin
Copy link
Member

URLs that do not specify a port number now sort as if they specified the scheme’s default port number. If none of the URLs in a given set specify it (which is the common case), I think the relative order is is unchanged.

@alexcrichton
Copy link
Contributor

Eh that seems fine. The ordering of URLs is already a little ambiguous anyway I think, so tweaking it a bit should be fine.

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.

None yet

6 participants