Skip to content

Conversation

@dustinhiatt-wf
Copy link
Contributor

CODE REVIEW

In order to make this package more generic (keys other than uint64) I modified the interfaces a bit so all they require is the Compare method. This means you could use skip lists to store strings, ints, uints, and random type but also kills us performance-wise. I would love generics here as the "byposition" and "byvalue" code shows that we are taking a 5x performance hit to make this code generic. I also killed the skiplist* type as it requires computable keys which only works if the key is computable (number types basically). We could re-introduce this with generics, if we ever get them.

@alexandercampbell-wf @beaulyddon-wf @tannermiller-wf @ericolson-wf @stevenosborne-wf @tylertreat-wf @rosshendrickson-wf

@alexandercampbell-wk
Copy link
Contributor

+1

@beaulyddon-wf
Copy link

But why would you ever need generics? ;p

+1

@tannermiller-wf
Copy link
Contributor

+1 Time to rewrite in a different language 😉

@alexandercampbell-wk
Copy link
Contributor

I got Go code to call Rust code a few days ago. It's actually not too difficult-- just compile Rust to a C library (easier than you think) and write headers so Go can recognize it.

@tannermiller-wf
Copy link
Contributor

@alexandercampbell-wf yeah I was playing with that in december. It's pretty smooth actually.

@alexandercampbell-wk
Copy link
Contributor

The biggest problem is that you're constantly casting your variables back and forth between Rust -> C -> Go and Go -> C -> Rust. Very awkward.

@tannermiller-wf
Copy link
Contributor

Yeah, that sucks, but it's a small price to pay.

dustinhiatt-wf added a commit that referenced this pull request Feb 9, 2015
@dustinhiatt-wf dustinhiatt-wf merged commit 941bf46 into master Feb 9, 2015
@alexandercampbell-wk alexandercampbell-wk deleted the skiplist_compare branch February 11, 2015 14:44
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.

5 participants