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

Allow URLs to be used as the keys of a TreeMap. #29

Closed
wants to merge 1 commit into from

Conversation

@cgaebel
Copy link

cgaebel commented Sep 16, 2014

This is needed to get bucketed profiling working in servo.

@SimonSapin
Copy link
Member

SimonSapin commented Sep 16, 2014

I don’t think Ord makes sense on URLs. Would it work to use url.to_string() as a key, instead?

@cgaebel
Copy link
Author

cgaebel commented Sep 16, 2014

Why wouldn't Ord make sense? And I can use url.to_string(), but it'd be less efficient...

@SimonSapin
Copy link
Member

SimonSapin commented Sep 16, 2014

How should you decide if an URL is "greater" or "lesser" than another? The implementation given by deriving is completely arbitrary.

@cgaebel
Copy link
Author

cgaebel commented Sep 16, 2014

Oh. I thought it'd be lexicographic.

@SimonSapin
Copy link
Member

SimonSapin commented Sep 16, 2014

It probably is lexicographic, but based on an internal representation that might change and does not say anything meaningful about the URLs.

@cgaebel
Copy link
Author

cgaebel commented Sep 16, 2014

Ok. Makes sense to me. Take another look?

@SimonSapin
Copy link
Member

SimonSapin commented Sep 17, 2014

Although marginally better, I’d still rather not have Url implement Ord at all.

But more importantly, using TreeMap<Url, ...> with an implementation of Ord that is based on Url::serialize will serialize a lot more often than using TreeMap<String, ...> with string keys from Url::serialize. I suggest doing the latter.

@cgaebel
Copy link
Author

cgaebel commented Sep 17, 2014

Ah. That's a great idea! I'll close this.

@cgaebel cgaebel closed this Sep 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.