-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Add and implement primitive list.copy() #18771
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
Conversation
implements the primitive list.copy() method
added list.copy()
creates a test for list.copy()
fixture for list.copy()
creates a test for list.copy() renamed for missing .test extension
fixture for list.copy()
mypyc/lib-rt/list_ops.c
Outdated
return PyList_GetSlice(list, 0, PyList_GET_SIZE(list)); | ||
} | ||
_Py_IDENTIFIER(copy); | ||
return _PyObject_CallMethodIdNoArgs(list, &PyId_copy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to get rid of _PyObject_CallMethodIdNoArgs
as it's only an internal method. See #18761 It would be better to use PyObject_CallMethodNoArgs
here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! The tests need to be moved to another location, but otherwise looks good.
test-data/unit/check-listcopy.test
Outdated
@@ -0,0 +1,31 @@ | |||
[case testListCopy] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mypy test case, which doesn't test the change made in this PR. Add test cases to mypyc/test-data/run-lists.test
and mypyc/test-data/irbuild-lists.test
instead, and remove this file. The irbuild test case only needs to test a single call to ensure that the new primitive is being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe test calling copy
on a subclass of list
, when the subclass instance has been assigned to a variable with list
type (so that the new primitive will be triggered).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a subclass I can use in the test? I'm not familiar with mypy and things didn't work out when I tried to make a subclass inside the test.
When running tests, I get native.py:2: error: Inheriting from most builtin types is unimplemented
error in run-lists.test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to define the subclass in a module that is not compiled. You can achieve this by putting the class definition in a separate file using [file interp.py]
in run-*.test
file. Example:
[case testSomething]
from interp import C
# this code will be compiled
[file interp.py]
# This file is not compiled, so you can do more things here
class C(list):
pass
test-data/unit/check-listcopy
Outdated
@@ -0,0 +1,31 @@ | |||
[case testListCopy] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a duplicate and can be removed.
test-data/unit/fixtures/list.pyi
Outdated
@@ -25,6 +25,7 @@ class list(Sequence[T]): | |||
def __setitem__(self, x: int, v: T) -> None: pass | |||
def append(self, x: T) -> None: pass | |||
def extend(self, x: Iterable[T]) -> None: pass | |||
def copy(self) -> list[T]: pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is again only targeting mypy (not mypyc) test cases. Please update mypyc/test-data/fixtures/ir.py
instead and revert this change.
This reverts commit eec3dfa.
tests and fixture for list.copy() moved to the correct locations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Add primitive for list.copy #1092