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

quicksort #209

Merged
merged 4 commits into from
Nov 9, 2016
Merged

quicksort #209

merged 4 commits into from
Nov 9, 2016

Conversation

stereosteve
Copy link
Contributor

No description provided.

@thejoshwolfe
Copy link
Contributor

Does this assume that strings can be compared with the < operator? I think this code needs a parameterized comparison function.

@phase
Copy link
Contributor

phase commented Nov 2, 2016

I believe it's comparing the bytes within the string with <.

@thejoshwolfe
Copy link
Contributor

ah. then is that test assuming that a string literal makes a writable buffer of bytes? i believe string literals are supposed to be read only. there should actually by a type check error when using a string literal as a []u8. a string literal is really a []const u8. i think you're getting away with it only because zig's const checking isn't strict enough yet.

@stereosteve
Copy link
Contributor Author

stereosteve commented Nov 2, 2016

@thejoshwolfe here is a version that has sortCmp for a custom compare function. Perhaps that should be the only version and the user must supply a cmp function.

@thejoshwolfe
Copy link
Contributor

@stereosteve cool. but why does that code have two sort functions? why not use the more general-purpose sortCmp for everything? If you don't want to provide a comparator, then the default should probably be:

pub fn operatorCompare(inline T: type, a: T, b: T) -> Cmp {
    return if (a > b) Cmp.Greater else if (a < b) Cmp.Less else Cmp.Equal;
}

And it seems like operatorCompare() should be added to the math.zig module, since it's pretty general purpose.

@stereosteve
Copy link
Contributor Author

stereosteve commented Nov 3, 2016

@thejoshwolfe yeah I was trying to do something like that, but couldn't figure out how to :(. Still a zig noob.

Like I changed sortCmp to:

pub fn sortCmp(inline T: type, array: []T, inline cmp: fn(inline T: type, a: T, b: T)->Cmp) {

and called it like:

sortCmp(i32, case[0], operatorCompare);

and compiler says:

./sort.zig:120:31: error: expected type 'fn(type, i32, i32) -> Cmp', found '(generic function)'
        sortCmp(i32, case[0], operatorCompare);

I tried a few other things as well, but to no avail... Are there anonymous functions in zig? Or is there a way to partially apply the Type parameter of operatorCompare, like operatorCompare.curry(i32) or something?

Perhaps the answer is to just require that the user alway provide a compare function.

@andrewrk
Copy link
Member

andrewrk commented Nov 3, 2016

error: expected type 'fn(type, i32, i32) -> Cmp', found '(generic function)'

Sounds like you have run into a bug in the compiler. I'll have a look.

Are there anonymous functions in zig?

Currently, no.

Or is there a way to partially apply the Type parameter of operatorCompare, like operatorCompare.curry(i32) or something?

Does this count?

fn operatorCompare_i32(a: i32, b: i32) -> Cmp {
    operatorCompare(i32, a, b)
}

One thing that may work, if the prototype of the sort function is:

pub fn quicksort(inline T: type, array: []T, inline cmp: fn(a: T, b: T)->Cmp) { 
    // ...
}

So use the T from the outer generic param instead of the comparator having its own generic type.

Provide some handy functions for builtin comparable types.
@stereosteve
Copy link
Contributor Author

Cool that all makes sense.

Okay, I updated sort to require a compare function, and included some handy functions for builtin comparable types. e.g.

sort(i32, case[0], i32asc);
sort(i32, case[0], i32desc);

Let me know if that's a good approach and if you like the naming. If so I'll add helper functions for the rest of the types.

@fsaintjacques
Copy link
Contributor

I'd like to know what is the priority of #130, because it will dictate the standard library implementation such as this.

@thejoshwolfe
Copy link
Contributor

@fsaintjacques i don't think we need to finalize this api just yet. as cool as it would be to implement a standard sort api that used some kind of comparable trait to decide how to compare elements, function pointers work just fine and are even slightly more powerful, at least as far as i understand the trait idea.

i think this PR should move forward with function pointers for now. when #130 happens we can change the standard library to use it.

@andrewrk andrewrk merged commit 1e0b493 into ziglang:master Nov 9, 2016
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