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

Change sort optional parameters from Handle to any and update SortFunc1D/2D typedef #1741

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JoinedSenses
Copy link
Contributor

@JoinedSenses JoinedSenses commented Apr 10, 2022

  • Change optional parameters from Handle to any; Not seeing any particular reason why they need to exclusively be a Handle.
  • Modify the SortFunc2D typedef.
    • Remove char typedef (char cannot be coerced to any).
    • Change typeset to typedef and make the parameters any instead of int
  • Change Sort(Custom/Func)1D params from int to any

Not seeing any particular reason why the optional parameter needs to exclusively be a handle.

This change shouldn't break any API.
@JoinedSenses JoinedSenses changed the title Change sort optional parameters from Handle to any Change sort optional parameters from Handle to any and update SortFunc2D typedef Apr 10, 2022
@JoinedSenses JoinedSenses changed the title Change sort optional parameters from Handle to any and update SortFunc2D typedef Change sort optional parameters from Handle to any and update SortFunc1D/2D typedef Apr 11, 2022
Copy link
Member

@Headline Headline left a comment

Choose a reason for hiding this comment

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

Other than that one doc comment, this seems fine to me!

Go ahead and switch that out and I'll pull this in

@peace-maker
Copy link
Member

any and floats aren't friends, since the float operators aren't used if a float is tagged as any. This could cause confusion when sorting float arrays.

Can you add a warning for float arrays?

@JoinedSenses
Copy link
Contributor Author

JoinedSenses commented May 4, 2022

any and floats aren't friends, since the float operators aren't used if a float is tagged as any. This could cause confusion when sorting float arrays.

Can you add a warning for float arrays?

This seems like a general sp nuisance not specific to sort stuff that folks should be aware of in general, so I feel like this isn't the best place to put that info, though I don't mind adding a note I suppose.

@Headline
Copy link
Member

Headline commented May 4, 2022

any and floats aren't friends, since the float operators aren't used if a float is tagged as any. This could cause confusion when sorting float arrays.

Can you add a warning for float arrays?

If they're retagged in the function prototype for comparison the correct operator will be used, no?

iirc you can pass in a float[] as any[], and in the comparison accept float type and not any

@JoinedSenses
Copy link
Contributor Author

any and floats aren't friends, since the float operators aren't used if a float is tagged as any. This could cause confusion when sorting float arrays.
Can you add a warning for float arrays?

If they're retagged in the function prototype for comparison the correct operator will be used, no?

iirc you can pass in a float[] as any[], and in the comparison accept float type and not any

yep, that is also true

Copy link
Member

@asherkin asherkin left a comment

Choose a reason for hiding this comment

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

LGTM

The SortCustom2D / SortFunc2D change is interesting because I'm pretty sure this used to work correctly (as long as the callback matched) before passing char[] to any[] was prevented. With that change and now this actually removing the unusable prototype I think we've lost the ability to sort arrays of strings.

@FortyTwoFortyTwo
Copy link
Contributor

My pull request at #1124 is very identical to this apart from mine having optional data param in callbacks.
No idea why my PR gets ignored, but this PR could be updated to have SortFunc(1D/2D/ADTArray) without having data param as optional.

@KyleSanderson
Copy link
Member

@asherkin are you still OK with this?

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.

6 participants