ext-ds icon indicating copy to clipboard operation
ext-ds copied to clipboard

Global compare FCI should be saved and restored (allowing nested comparators)

Open rtheunissen opened this issue 7 years ago • 10 comments

All structures share the same global FCI, so if while sorting something you try to sort something else, things will break. The best way to handle this is to save the current FCI, use it, then restore it.

rtheunissen avatar Aug 30 '16 05:08 rtheunissen

Why does this need a global at all?

nikic avatar Aug 30 '16 06:08 nikic

Because we need to call a user function inside a comparator (C function pointer).

rtheunissen avatar Aug 30 '16 07:08 rtheunissen

Why can't it be passed along as a context pointer? As you aren't using zend_hash_sort, you shouldn't be limited by a context-free comparator.

nikic avatar Aug 30 '16 07:08 nikic

I don't believe qsort supports a context parameter.

rtheunissen avatar Aug 30 '16 07:08 rtheunissen

Huh, you're right. I could have sworn there was a qsort_ex variant, but looks like there isn't :(

nikic avatar Aug 30 '16 07:08 nikic

There is, but it's not standard. I wonder how plausible it is to implement quick sort manually.

rtheunissen avatar Aug 30 '16 20:08 rtheunissen

There are two variants commonly found: qsort_r and qsort_s. Sadly the APIs aren't quite the same even though they are supporting the same idea: add a context parameter.

Perhaps we could use this: https://github.com/noporpoise/sort_r. It is in the public domain, so no issue there.

morrisonlevi avatar Dec 02 '16 21:12 morrisonlevi

@morrisonlevi interesting idea, would be very easy to just nab that header file and include it. I like.

rtheunissen avatar Dec 02 '16 22:12 rtheunissen

Coming back to this 2 years later, do we think that a third party sort is still the way to go?

rtheunissen avatar Mar 14 '19 14:03 rtheunissen

@morrisonlevi the sort_r lib you linked falls back to their own quicksort implementation, but uses qsort_r if it is available. Seems reasonable to me to just include that header then. Might look for some others too..

rtheunissen avatar Mar 14 '19 14:03 rtheunissen