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

Implement std::borrow::Borrow for Atom #89

Closed
wants to merge 1 commit into from

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Jul 4, 2015

I want to be able to use connect() on Atoms, which requires
Borrow be implemented

I want to be able to use connect() on Atoms, which requires
Borrow<str> be implemented
@frewsxcv frewsxcv force-pushed the frewsxcv:atom-borrow branch from 20e73f3 to 636d79f Jul 4, 2015
@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2015

I think this would potentially break hashmaps, but I don’t know how much of a problem that is in practice.

HashMap<K, V> has methods like:

fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V> where K: Borrow<Q>, Q: Hash + Eq

which, as far as I understand, assume that the Hash impls of K and Q are compatible. But hashing of Atom is not based on the derefed str, it’s derived and thus based only on the internal u64. This makes hashing fast, in the same way that comparison is fast. I don’t know if Servo ever hashes atoms, though.

@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2015

Also, SliceConcatExt (which defines connect) should probably rely on AsRef rather than Borrow: rust-lang/rust#26780

@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 5, 2015

I think this would potentially break hashmaps

Very good point. I didn't even think about this as a side effect.

Also, considering the behavior might be changing soon (and the rename from connect to join), I'll close this and use this pattern for the time being.

@frewsxcv frewsxcv closed this Jul 5, 2015
@frewsxcv frewsxcv deleted the frewsxcv:atom-borrow branch Jul 5, 2015
@SimonSapin
Copy link
Member

SimonSapin commented Jul 5, 2015

It’s not as pretty, but something like this would avoid allocating the intermediate vector of that pattern:

let mut iter =  atoms.iter();
if let Some(first) = iter.next() {
    iter.fold(String::new(first), |s, atom| { s.push_str(" "); s.push_str(atom) })
} else {
    String::new()
}
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jul 5, 2015
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jul 5, 2015
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jul 7, 2015
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.