To me, it would feel more logical if sort_loop had the ArrayMethod signature rather than that of the (hopefully eventually removed) sort loop.

But obviously that would need to have a standard array method loop that uses a comparison function -- see main comment.

Yes, it would be better, and also it seems to be a very useful feature if exposed - dtypes do not need to restrict themselves to sort_compare or similar functionality if they need to do setup work etc. Seems like a natural extension if it's okay in a follow-up; I just tried to keep this PR minimal!

Nit: keep starting brace on its own line.

This is done, thanks!

The name confused me, since it suggested filling an array. Maybe just get_sort_data_from_array?

Though since it is only two lines, I also think it is also fine to repeat those 4 times in the code - it helps understanding why it is needed!

Hmm, the main idea was just to have it reusable along with a fill_sort_data_with_context for new arraymethod-style loops - in case we need to change some logic, we just change the helper. But I can see why this is a bit too minimal not to duplicate, and the sorts are legacy anyway - @ngoldbaum since you raised performance concerns with this, would you prefer just duplicating?

I don't think you get any speed benefit from explicitly in-lining, since the function is marked as inline, presumably the compiler will do that already. My main complaint really was about the name -- but if there is precedent in the same file, feel free to ignore that!

OK! There is no precedent - I've renamed the function. Thank you!

Since the two loops that we return are identical except for passing on different NpyAuxData, I think we can simplify this to have just one loop, but already set the auxdata here (a nice aspect of what get_loop can do!).

Sorry, I'm not sure I fully understand! We don't pass different transferdata? Do you mean we can use NpyAuxData to pass the legacy sort loop? Where would that go?

I can see using context->parameters inside the sort loop, instead of get_loop, though! That sounds like a nice idea if that's what you meant.

Sorry that I wasn't very. Basically, I think you can do,

stringdtype_sort_loop(  // Just one sort loop!
        PyArrayMethod_Context *context,
        char *const *data, const npy_intp *dimensions, const npy_intp *strides,
        NpyAuxData *transferdata)
{
    return stringdtype_wrap_sort_loop(
            context, data, dimensions, strides, transferdata,
            transferdata);  // transferdata contains npy_aquicksort_impl or npy_amergesort_impl
}

int
stringdtype_get_sort_loop(
        PyArrayMethod_Context *context,
        int aligned, int move_references,
        const npy_intp *strides,
        PyArrayMethod_StridedLoop **out_loop,
        NpyAuxData **out_transferdata,
        NPY_ARRAYMETHOD_FLAGS *flags)
{
    PyArrayMethod_SortParameters *parameters = (PyArrayMethod_SortParameters *)context->parameters;
    *flags |= NPY_METH_NO_FLOATINGPOINT_ERRORS;
    if (parameters->flags == NPY_SORT_STABLE) {
        *out_transferdata = &npy_amergesort_impl;
    else if (parameters->flags == NPY_SORT_DEFAULT) {
        *out_transferdata = &npy_aquicksort_impl;
    else {
        PyErr_SetString(PyExc_RuntimeError, "unsupported sort kind");
        return -1;
    }
    *out_loop = (PyArrayMethod_StridedLoop *)stringdtype_sort_loop;
    return 0;
}

Sorry if I am missing something obvious, but would this not cause a pointer issue (casting &npy_amergesort_impl to NpyAuxData *, for example, cannot be done)? Should we define a new struct with the auxdata base to cast to and back is what you mean? Happy to do that of course!

Edit: tried what you sent, and indeed there was a cast issue, at least the function cannot be directly called with the signature then...

Yes, it seems using NpyAuxData requires a struct definition with an auxdata base, so something like:

typedef struct {
    NpyAuxData base;
    PyArray_SortImpl *sort_impl;
} StringDTypeSortingAuxData;

with free and clone might work?

I don't think you need a struct...Note that you are using auxdata to pass the function in the code you have above as well, just a layer deeper.

Grepping, I see that in dtype_transfer.c, the same kind of thing is done, but it does have a cast:

That said, maybe this does behave differently than the auxdata in legacy-style loops: it seems everywhere I can find it, memory is allocated for it. Let's ping @seberg, since that may just be because everywhere else there is actually an allocation needed. Unfortunately, the documentation does not quite help -- https://numpy.org/doc/stable/reference/c-api/array.html#c.PyArrayMethod_GetLoop

Yes, memory probably needs to be allocated. Even in that example though, there is a struct, which is the type of data:

And after that descr and move_references are members of the struct.

Note that you are using auxdata to pass the function in the code you have above as well, just a layer deeper.

Maybe you are looking at the (arg)sort_loop parameter? There is an additional argument after transferdata:

int
stringdtype_wrap_sort_loop(
        PyArrayMethod_Context *context,
        char *const *data, const npy_intp *dimensions, const npy_intp *strides,
        NpyAuxData *transferdata, PyArray_SortImpl *sort_loop)

@MaanasArora - Now had time to look a bit more carefully and with out_transferdata requiring allocation, it is just too much work. It is also not really necessary: if we move selection of the implementation in the wrapping loops, things can stay super simple. I just made a PR to your PR to implement that - have a look!

Great, thanks - I've merged your PR!

I think we need only one loop (say, stringdtype_sort_loop and stringdtype_argsort_loop if we use transferdata -- see comment on get_loop.

Same as above!

Small one, but I think by setting *view_offset = 0 you actually indicate it can be a view (no view is NPY_MIN_INTP. I think it is fine not to touch view_offset at all, but maybe best to ask @seberg.

Ah right, perhaps not good to touch it, thanks. I'll delete these lines, I think.

Deleted the lines!

NIT:

(and elsewhere). We are not hiding symbols by default yet, I think.

Thanks, this is done!