[cython/cython] [ENH] Change CyFunction defaults struct to cdef classes (#4374)

[da-woods]

Currently, non-constant default values on CyFunctions are stored in a struct and accessed through a void pointer together with a couple of housekeeping values needed to use it:

https://github.com/cython/cython/blob/aea4e6b84b38223c540266f8c57093ee2039f284/Cython/Utility/CythonFunction.c#L55-L57

(We then store the introspectable __defaults__ and __kwdefaults__ as separate PyObjects*).

This works, but requires multiple awkward loops over it in a few places

https://github.com/cython/cython/blob/aea4e6b84b38223c540266f8c57093ee2039f284/Cython/Utility/CythonFunction.c#L625-L626
https://github.com/cython/cython/blob/aea4e6b84b38223c540266f8c57093ee2039f284/Cython/Utility/CythonFunction.c#L668-L669
https://github.com/cython/cython/blob/aea4e6b84b38223c540266f8c57093ee2039f284/Cython/Utility/CythonFunction.c#L1258-L1259

I'd like to replace this struct with a cdef class, generated in a similar way to the closure class. My view is:

Advantages

1. It simplifies the implementation of CyFunction - it keeps track of one PyObject* for the defaults and everything else is handled automatically using the tried-and-tested code that handles our cdef classes.
2. It makes it much easier to pickle the defaults - #4368 can't currently handle this. (This is the immediate reason I was thinking about it).
3. It potentially makes it easier for the introspectable __defaults__ and __kwdefaults__ to be linked back to the stored defaults - #2650. The logic to do this could be on the cdef class, which could act as a dictionary-like proxy for example. I wouldn't propose to do this on the initial implementation but it's easier to see how to do it in future.
4. There are a few bugs in the current implementation relating to c++ classes, which are mainly just memory leaks, but could also be crashes:

   # distutils: language=c++
   
   from libcpp.vector cimport vector
   
   # neither the constructor nor destructor of the default vector are ever called
   # surprisingly this doesn't actually crash it on my PC, but this is pure luck...
   def f(vector[int] a=[1,2,3]):
       return sum(ai for ai in a)
   
   from cython cimport floating
   
   class C:
       # binding of fused functions causes the defaults to be copied by memcpy
       # which is absolutely invalid. This only avoids problems because the
       # destructor is never called...
       def f(self, floating dummy, vector[int] a=[1,2,3]):
           return sum(ai+dummy for ai in a)

   cdef class already has the infrastructure to get this right.

Disadvantages

Generating a new (internal) cdef class does require more code than generating a struct, so it probably gives larger C files and increases compilation times.

Additional context

I'm not planning to do this right away, but I wanted to post it to give people opportunity to tell me that I'm wrong. It's possible that the cython-devel list might have been the better place to post it

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

[da-woods]

Closed #4374 as completed.

—

Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.Message ID: <cython/cython/issue/4374/issue_event/12882680640@github.com>