Use a more compatable way to get wrapper_descriptor and method_wrapper#30
Open
Daetalus wants to merge 1 commit intotesting-cabal:masterfrom
Open
Use a more compatable way to get wrapper_descriptor and method_wrapper#30Daetalus wants to merge 1 commit intotesting-cabal:masterfrom
Daetalus wants to merge 1 commit intotesting-cabal:masterfrom
Conversation
The original code use type(type.__call__) to get the type `wrapper_descriptor` and use `type(all.__call__)` to get type `method_wrapper`. according the comment in here: https://github.com/Daetalus/funcsigs/blob/master/funcsigs/__init__.py#L170. It could avoid infinite recursive or sefault. In Pyston, the type of `type.__call__` is `instancemethod`. So it will cause segfault. I change it to `type(bytearray.__add__)`, because in Pyston, its type is `wrapper_descriptor`, this is also work in CPython 2.x and CPython 3.x. More information, please see scikit-learn/scikit-learn#7162.
| _WrapperDescriptor = type(type.__call__) | ||
| _MethodWrapper = type(all.__call__) | ||
| _WrapperDescriptor = type(bytearray.__add__) | ||
| _MethodWrapper = type(u'str'.__add__) |
There was a problem hiding this comment.
Why not plain 'str'.__add__? u'' introduces an incompatibility with Python 3.2
Author
There was a problem hiding this comment.
Emm... This is because in Pyston, the type of 'str'.__add__ still is instancemethod.
Pyston v0.6.0 (rev 8dd1298946b5dbe2b28f75a7a6d2de6486765438), targeting Python 2.7.7
>> type('str'.__add__)
<type 'instancemethod'>
>> type(u'str'.__add__)
<type 'method-wrapper'>
And according the description of funcsigs in README file, I thought we can drop the Python 3.2 support.
Author
|
Guys? @epsy @msabramo @rbtcollins Does there has a better way to do this? |
|
(I'm only a user of funcsigs, I don;t have merge approval access) I suppose it's entirely fair to drop Python 3.2 support at this point. |
epsy
approved these changes
Sep 15, 2016
Author
|
Thanks for review it! @epsy |
|
ping @rbtcollins maybe. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The original code use type(type.call) to get the type
wrapper_descriptorand usetype(all.__call__)to get typemethod_wrapper. according the comment in here:https://github.com/Daetalus/funcsigs/blob/master/funcsigs/__init__.py#L170.
It could avoid infinite recursive or sefault.
In Pyston, the type of
type.__call__isinstancemethod. So it willcause segfault. I change it to
type(bytearray.__add__), because inPyston, its type is
wrapper_descriptor, this is also work in CPython2.x and CPython 3.x.
More information, please see scikit-learn/scikit-learn#7162.