Skip to content
This repository has been archived by the owner on Jul 24, 2023. It is now read-only.

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

Merged
merged 2 commits into from
Jun 15, 2019
Merged

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

merged 2 commits into from
Jun 15, 2019

Conversation

icholy
Copy link
Contributor

@icholy icholy commented Jun 13, 2019

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
}

Copy link
Owner

@sbinet sbinet left a comment

Choose a reason for hiding this comment

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

alternatively, and to make it behave more like the C API, one could modify togo to check the ptr pointer for C.Py_None and return python.Py_None.
https://github.com/sbinet/go-python/blob/master/object.go#L36

WDYT?

python_test.go Outdated Show resolved Hide resolved
@icholy
Copy link
Contributor Author

icholy commented Jun 14, 2019

That's a way better solution.

@icholy
Copy link
Contributor Author

icholy commented Jun 14, 2019

We should probably do the same with Py_True and Py_False

@sbinet
Copy link
Owner

sbinet commented Jun 14, 2019

SGTM.

@icholy
Copy link
Contributor Author

icholy commented Jun 14, 2019

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
Copy link
Contributor Author

icholy commented Jun 14, 2019

I went with the 2 functions

@icholy icholy changed the title tests: add failing test for None check object: always return the same value for Py_None, Py_True, and Py_False Jun 14, 2019
@sbinet
Copy link
Owner

sbinet commented Jun 14, 2019

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
Copy link
Contributor Author

icholy commented Jun 14, 2019

no problem

@icholy
Copy link
Contributor Author

icholy commented Jun 14, 2019

@sbinet should be good to go

@sbinet sbinet merged commit 46d882b into sbinet:master Jun 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants