| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Please use 4 spaces to ident your code (as the rest of _asynciomodule.c file).
Sorry, something went wrong.
There was a problem hiding this comment.
Move ); to the previous line, so that
int class_has_attr = _PyObject_HasAttrId(
class, &PyId__asyncio_future_blocking);
Sorry, something went wrong.
There was a problem hiding this comment.
While this code is OK, you shouldn't use refs to Python objects after you DECREF them, it's a bad style. Please compare first, and decref after,
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
I have made the requested changes; please review again. |
Sorry, something went wrong.
|
Thanks for making the requested changes! @1st1: please review the changes made to this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
Why not use Future_CheckExact() / Future_Check() as very fast happy path here?
Sorry, something went wrong.
There was a problem hiding this comment.
Good idea.
Sorry, something went wrong.
There was a problem hiding this comment.
@asvetlov @1st1 isfuture check if _asyncio_future_blocking exists and is not None because mock object has _asyncio_future_blocking as None and we want to return isfuture() as False in this case.
See test case
cpython/Lib/test/test_asyncio/test_futures.py
Lines 123 to 124 in 8425de4
Do we really want to use Future_CheckExact to return fast?
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
I don't know whether it is worth to add C implementation of this function, but I think it can be simplified.
See also that the _asyncio_future_blocking attribute is used in the existing C code in suboptimal way.
Sorry, something went wrong.
There was a problem hiding this comment.
It is better to use Py_TYPE() for convenience and speed . There is a subtle difference between the type and the __class__ attribute, but it is usually ignored when write C accelerations.
Sorry, something went wrong.
There was a problem hiding this comment.
I suggested to @jimmylai to get the class attribute to make sure that the C implementation behaves as the Python implementation: see PEP 399. If someone wants to use type(), I would prefer to see the same change in the Python implementation as well.
@1st1, @asvetlov: Do you know the rationale for checking class here?
Sorry, something went wrong.
There was a problem hiding this comment.
I suggested to use Py_TYPE as well.
I think it was I who used __class__ there, and there's no rationale behind that :)
Sorry, something went wrong.
There was a problem hiding this comment.
So the Python version can be updated to use type() too.
Sorry, something went wrong.
There was a problem hiding this comment.
@1st1 @serhiy-storchaka
use type() won't work.
Original isfuture: return (hasattr(obj.__class__, '_asyncio_future_blocking') and obj._asyncio_future_blocking is not None)
Use type: return (hasattr(type(obj), '_asyncio_future_blocking') and obj._asyncio_future_blocking is not None)
Unit tests fail when use type()
Did I use type() wrong?
======================================================================
FAIL: test_isfuture (test.test_asyncio.test_futures.CFutureTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/jimmylai/workspace/cpython/Lib/test/test_asyncio/test_futures.py", line 131, in test_isfuture
self.assertTrue(asyncio.isfuture(mock.Mock(type(f))))
AssertionError: False is not true
======================================================================
FAIL: test_isfuture (test.test_asyncio.test_futures.CSubFutureTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/jimmylai/workspace/cpython/Lib/test/test_asyncio/test_futures.py", line 131, in test_isfuture
self.assertTrue(asyncio.isfuture(mock.Mock(type(f))))
AssertionError: False is not true
======================================================================
FAIL: test_isfuture (test.test_asyncio.test_futures.PyFutureTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/jimmylai/workspace/cpython/Lib/test/test_asyncio/test_futures.py", line 131, in test_isfuture
self.assertTrue(asyncio.isfuture(mock.Mock(type(f))))
AssertionError: False is not true
Sorry, something went wrong.
There was a problem hiding this comment.
Use 4-space indentation.
Sorry, something went wrong.
There was a problem hiding this comment.
fixed.
Sorry, something went wrong.
There was a problem hiding this comment.
Make the argument positional-only.
Sorry, something went wrong.
There was a problem hiding this comment.
In pratice, it means adding "/" on a new line, aligned with "obj", and run "make clinic" again.
Sorry, something went wrong.
There was a problem hiding this comment.
Is it needed?
Sorry, something went wrong.
There was a problem hiding this comment.
class may not have attr _asyncio_future_blocking
It's needed.
Sorry, something went wrong.
There was a problem hiding this comment.
obj_attr is leaked here.
Sorry, something went wrong.
There was a problem hiding this comment.
added Py_DECREF(obj_attr);
Sorry, something went wrong.
|
I have made the requested changes; please review again. |
Sorry, something went wrong.
|
@jimmylai the PR is failing on tests passing. |
Sorry, something went wrong.
|
@jimmylai, please take a look at the last comment and please also resolve the merge conflict. Thank you! |
Sorry, something went wrong.
|
This PR is awaiting changes for over two years, tests were failing and has merge conflicts so I am closing it. If you are still interested you can create a new PR with the requested changes or this one can be reopened if needed. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
https://bugs.python.org/issue33521