sbinet / go-python

naive go bindings to the CPython2 C-API
Other
1.52k stars 138 forks source link

object: always return the same value for Py_None, Py_True, and Py_False #87

Closed icholy closed 5 years ago

icholy commented 5 years ago

https://docs.python.org/2/c-api/none.html

You're supposed to be able to compare a python.PyObject to python.Py_None but this doesn't work.

I think we should consider adding a PyNone_Check function to compare the underlying pointer.

// Return true if self is None
func PyNone_Check(self *PyObject) bool {
    return Py_None.ptr == self.ptr
}
icholy commented 5 years ago

That's a way better solution.

icholy commented 5 years ago

We should probably do the same with Py_True and Py_False

sbinet commented 5 years ago

SGTM.

icholy commented 5 years ago

There's an initialisation loop:

var Py_None = togo(C._gopy_pynone())

func togo(obj *C.PyObject) *PyObject {
    switch obj {
    case nil:
        return nil
    case Py_None.ptr:
        return Py_None
    case Py_True.ptr:
        return Py_True
    case Py_False.ptr:
        return Py_False
    default:
        return &PyObject{ptr: obj}
    }
}
# github.com/sbinet/go-python
./none.go:9:5: initialization loop:
    /home/icholy/go/src/github.com/sbinet/go-python/none.go:9:5 Py_None refers to
    /home/icholy/go/src/github.com/sbinet/go-python/object.go:36:6 togo refers to
    /home/icholy/go/src/github.com/sbinet/go-python/none.go:9:5 Py_None

We can either not use the togo function for initialising Py_None, Py_True, and Py_False.

var Py_None = &Object{C._gopy_pynone()}
// etc ...

or split togo into two functions.

func togo(obj *C.PyObject) *PyObject {
    switch obj {
    case Py_None.ptr:
        return Py_None
    case Py_True.ptr:
        return Py_True
    case Py_False.ptr:
        return Py_False
    default:
        return newgo(obj)
    }
}

func newgo(obj *C.PyObject) *PyObject {
    if obj == nil {
        return nil
    }
    return &PyObject{ptr: obj}
}

var Py_None = newgo(C._gopy_pynone())

what do you think?

icholy commented 5 years ago

I went with the 2 functions

sbinet commented 5 years ago

I must say I am unsure about the approach of these 2 very similarly-looking functions that do almost the same thing but not quite.

I think it would be as simple to leave only one function (togo) and initialize the 3 global variables like so:

var Py_None = &PyObject{ptr: C._gopy_none()}
var Py_False = &PyObject{ptr: C._gopy_pyfalse()}
var Py_True = &PyObject{ptr: C._gopy_pytrue()}
icholy commented 5 years ago

no problem

icholy commented 5 years ago

@sbinet should be good to go