Some small fixings #1398
Some small fixings #1398
Conversation
| Py_XDECREF(self); | ||
| Py_INCREF(instance); | ||
| ((BoxedInstanceMethod*)im)->obj = instance; | ||
| Py_INCREF(im); |
There was a problem hiding this comment.
I think the pattern is usually
Box* old = o->attr;
Py_INCREF(new_attr);
o->attr = new_attr;
Py_DECREF(old);
ie you want to decref the old value after replacing it, not before. I also don't think we need the Py_INCREF(im) call here.
| @@ -2381,6 +2386,7 @@ void setupBuiltins() { | |||
| = BoxedClass::create(type_cls, object_cls, 0, 0, sizeof(Box), false, "ellipsis", false, NULL, NULL, false); | |||
| ellipsis_cls->giveAttr("__repr__", | |||
| new BoxedFunction(BoxedCode::create((void*)ellipsisRepr, STR, 1, "ellipsis.__repr__"))); | |||
| ellipsis_cls->tp_repr = ellipsis_repr; | |||
There was a problem hiding this comment.
The proper fix here is to call ellipsis_cls->freeze() (and get rid of ellipsis_repr), which will automatically fill in tp_repr for us.
| @@ -4405,7 +4403,7 @@ void setupRuntime() { | |||
| type_cls->giveAttr( | |||
| "__new__", new BoxedFunction(BoxedCode::create((void*)typeNewGeneric, UNKNOWN, 4, false, false, "type.__new__"), | |||
| { NULL, NULL })); | |||
| type_cls->giveAttr("__repr__", new BoxedFunction(BoxedCode::create((void*)typeRepr, STR, 1, "type.__repr__"))); | |||
There was a problem hiding this comment.
What code relies on the type of type.__repr__? This change is a bit scary because it could affect a lot of things.
There was a problem hiding this comment.
'instancemethodwill store theim_self(akaobjinBoxedInstanceMethod. In some situation, it will causestr(wrapped)andrepr(wrapped)` returns different result.
Daetalus
force-pushed
the
small_fixings
branch
5 times, most recently
from
711f193 to
03b8197
Compare
field Some extensions, like Zope2.ExtensionClass(PyECMethod_New_ function), will change instance method's im_self field in some situation. They do it by access the field of PyMethodObject directly. We can't do it, so provide a new API to do it.
Daetalus
force-pushed
the
small_fixings
branch
2 times, most recently
from
d5c25a9 to
098013c
Compare
|
@undingen Ready to review. :) |
BST_ImportName This commit is a preperation of the commit which for add co_names field. BST_StoreName and some other nodes use `index_id` and InteredString to store variable name. But BST_ImportName use vreg and vreg_name. We can't get the module name inside code object by vreg. So change it to the way which BST_StoreName used.
There was a problem hiding this comment.
I think except of the last commit (Add PyObject_GetDict and other two APIs) the changes look good :-)
I added some general comments to the last one but unfortunately I don't know enough about our whole instance attr implementation to be sure your change is correct. @kmod could you please take a look at it 098013c
| if (!tp->instancesHaveHCAttrs()) { | ||
| if (!tp->instancesHaveDictAttrs()) | ||
| return NULL; | ||
| return obj->getDict(); |
There was a problem hiding this comment.
I think this will not always return a copy of the dict (the obj->getDict(); case)
How do you feel about naming it PyObject_GetDictCopy? Or does some other python implementation already use PyObject_GetDict?
There was a problem hiding this comment.
Only Jython has a public getDict API for different class.
There was a problem hiding this comment.
thanks for changing the name but I think this should call PyDict_Copy(obj->getDict()) because otherwise it's not always a copy as the name suggests...
|
|
||
| // This API just add or update key-value pairs. It will not remove existed items | ||
| // if the key is not contained in the new dict. | ||
| extern "C" void PyObject_SetDict(PyObject* obj, PyObject* dict) noexcept { |
There was a problem hiding this comment.
What about naming it PyObject_UpdateDict?
| Py_INCREF(d_key); | ||
| PyString_InternInPlace(&d_key); | ||
| Py_DECREF(d_key); | ||
| } |
There was a problem hiding this comment.
Look like to me like this block is not necessary, PyObject_SetAttr should already do the interning
|
Oh yes Marius is right -- there are two issues with |
Daetalus
force-pushed
the
small_fixings
branch
2 times, most recently
from
eb1d7ff to
026c988
Compare
Pyston can not support _PyObject_GetDictPtr properly. So add PyObject_GetDictCopy, PyObject_ClearDict, PyObject_UpdateDict API. The PyObject_GetDictCopy only return an copy of object's dict, not a writable dict pointer like _PyObject_GetDictPtr. The PyObject_UpdateDict only update or append new key-value pairs from new dict. It will not reset object's dict if the key is not existed in the new dict.
|
👍 thanks for the patch! |
No description provided.