New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python3 port of the C-Extension #9

Closed
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@stephan-hof
Copy link
Member

stephan-hof commented Sep 30, 2016

As requested in #7 I open a pull request to port the C-Extension of ExtensionClass.

stephan-hof added some commits Sep 22, 2016

Rewrite Base_getattro
Basically do what python version does.
Call object.__getattribute__ and if it's a descriptor for something besides __parent__, call it.
Drop support for python < 2.6.
Py_TPFLAGS_HAVE_VERSION_TAG is available since python2.6
Use PyVarObject_HEAD_INIT.
This macro works for python2 and python3.
Rewrite inheritedAttribute
Basically do the same as the python version.
Get the super class and search for the attribute there.
Copy pickle support from 'zopefoundation/persistent'
pickle support in persistent is already python3 ready so reuse the
implementation.
The python version is also a copy of persistent and the previous C version look
also like a copy&paste from persistent.
Improve the method check in EC_init.
In python3 unbound method disappeared.
So it is very likely that __class_init__ is just a function.
=> Allow functions also as __class_init__
Replace PyString_AsString with convert_name.
convert_name takes care to convert unicode (python3) names into PyBytes.
In python2 convert_name just checks if it's already a PyString.
(PyBytes and PyString are the *same* in python2)

So EC_setattro can rely on convert_name to emit always PyBytes.
Port the 'rebouncing' of methods when a new extension class is created.
The fact that unbound methods do not exist in python3 anymore make this a bit hard.
Basically I cannot call 'callable->ob_type->tp_descr_get' anymore, because the
callable has *no* im_class anymore.

Yes I could have done it in the python2 case, but from my point of view it should
behave consistent by not calling 'tp_descr_get' in in both versions.
A relative import is needed in python3.
Otherwise the import of python3 (which is absolute now), does not pick up the
C-Extension.
Refactor the name check of setattro into a separate function.
This makes it easier to Py_DECREF(as_bytes) in case of errors.
@tseaver

This comment has been minimized.

Copy link
Member

tseaver commented Oct 1, 2016

@stephan-hof Wow, thank you! I will try to get a review done this coming week (unless somebody else beats me to it).

@tseaver

This comment has been minimized.

Copy link
Member

tseaver commented Oct 2, 2016

I haven't done a detailed review, but I did check that your branch shows 100% test coverage after merging with the current master (we're only measuring coverage of Python code, of course).

@stephan-hof

This comment has been minimized.

Copy link
Member

stephan-hof commented Oct 4, 2016

I found a regression in ExtensionClass (python version and also my C version) while working on the C-Extension of Acquisition

The old C code of ExtensionClass changed an attribute error into the following:

class Dummy(Base):
    pass

Dummy().a
AttributeError: a

The python version (+ my ported C version) does the following:

class Dummy(Base):
    pass

Dummy().a
AttributeError: 'Dummy' object has no attribute 'a'

See, the error messages contains the typename of an object.
From my point of view a port should not change behaviour, so I would change the C and python version to hide the typename again.

The python version of Acquisition struggled also with the changed behaviour of ExtensionClass.
A lot of doctests there have asserts for AttributeError: <name> (no typename in the error message), but instead of questioning ExtensionClass a fix to Acquisition was introduced.
https://github.com/zopefoundation/Acquisition/blob/master/src/Acquisition/__init__.py#L745
When ExtensionClass behaves the old way Acquisition could be changed to rely on ExtensionClass to throw the appropriate AttributeError. This would make a port of the C-Extension of Acquisition easier too.

Alternative:

  • Keep the regression => Keep the new style Attribute error
  • Change all the doctests in Acquisition to assert the new style Attribute error
  • Change python version of Acquisition to keep the new style Attribute error

@jamadden any comments on this? Since you wrote the python version of Acquisition.

@jamadden

This comment has been minimized.

Copy link
Member

jamadden commented Oct 4, 2016

I don't particularly care. The only reason the code in Acquisition you pointed to is there is because of doctests; I'm not particularly a fan of doctests or relying on exact exception messages in general, but I didn't want to break anything or create too large of a PR.

Personally, I'd rather see the more informative error messages raised everywhere (although in Acquisition's case, that could be slightly misleading).

@tseaver

This comment has been minimized.

Copy link
Member

tseaver commented Oct 4, 2016

We have no guarantee that Python itself doesn't break reliance on exact exception message tests: feel free to break doctests which rely on them if if makes the code clearer / faster / more maintainable (but open issues against them, if you can).

@icemac
Copy link
Member

icemac left a comment

Thank you for your hard work. I do not know enough C to be able to review the changes.

@@ -1,6 +1,6 @@
[tox]
envlist =
py27,py27-pure,py33,py34,py35,pypy,pypy3,coverage
py27,py27-pure,py33,py34,py34-pure,py35,pypy,pypy3,coverage

This comment has been minimized.

@icemac

icemac Nov 8, 2016

Member

Is there a reason to test this specific Python 3 version as pure python?

This comment has been minimized.

@stephan-hof

stephan-hof Nov 8, 2016

Member

There is no specific reason why I have chosen this version. My goal was that at least one python3 version tests the python only implementation.
I think for completeness I would make sense to add a also a py33-pure and py35-pure possibly 3.6. I'll update the pull request in the next days.

Background:
Per default a test uses the C-Extension. In a pure environment the C-Extension is not compiled nor considered for the unittests. So only the pure environments test the python only implementation of ExtensionClass. Which means with the current envlist the python only implementation is tested in python27 and python34.

hannosch added a commit that referenced this pull request Jan 18, 2017

Port the C extensions to Python 3.
Merge pull request #9 from axiros/ExtensionClass/master.
@hannosch

This comment has been minimized.

Copy link
Member

hannosch commented Jan 18, 2017

I've reviewed this to the best of my ability and found no issues.

I've squashed and merged this manually in f6ef36a. In subsequent commits I've extended the tox test list of cover all combinations of pure/non-pure environments and all Python versions addressing icemac's comment.

@hannosch hannosch closed this Jan 18, 2017

@tseaver

This comment has been minimized.

Copy link
Member

tseaver commented Jan 18, 2017

@hannosch Thanks for picking up my slack!

@stephan-hof Thanks again for your patch!

@hannosch

This comment has been minimized.

Copy link
Member

hannosch commented Jan 18, 2017

@tseaver you are welcome :) @stephan-hof if you want to tackle that Acquisition C port now ... ;)

@stephan-hof

This comment has been minimized.

Copy link
Member

stephan-hof commented Jan 19, 2017

@hannosch many thanks for reviewing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment