FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-11572: Make several minor improvements to copy module by berkerpeksag · Pull Request #207 · python/cpython · GitHub

/ cpython Public

bpo-11572: Make several minor improvements to copy module #207

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.

Sign up for GitHub

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

Closed
berkerpeksag wants to merge 1 commit into from
Conversation 26 Commits 1 Checks 0 Files changed
Closed

bpo-11572: Make several minor improvements to copy module #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
29 changes: 9 additions & 20 deletions Lib/copy.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -75,24 +75,20 @@ def copy(x):
if copier:
return copier(x)

try:
issc = issubclass(cls, type)
except TypeError: # cls is not a class
issc = False
if issc:
if issubclass(cls, type):
# treat it as a regular class:
return _copy_immutable(x)

copier = getattr(cls, "__copy__", None)
if copier:
if copier is not None:

serhiy-storchaka Feb 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do you know that if copier is not None is slower than if copier?

But the change LGTM.

berkerpeksag Jun 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I didn't know that. How much slower than if foo?

serhiy-storchaka Jul 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not too much. But I think that having only None special is better that depending on the boolean value of the method. LGTM.
return copier(x)

reductor = dispatch_table.get(cls)
if reductor:
if reductor is not None:
rv = reductor(x)
else:
reductor = getattr(x, "__reduce_ex__", None)

serhiy-storchaka Feb 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

__reduce_ex__ could be absent in old style classes in Python 2. New style classes inherit it from object. Therefore this can be just x.__reduce_ex__.

berkerpeksag Jun 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Changing this line to x.__reduce_ex__ makes test_copy_cant fail:

======================================================================
ERROR: test_copy_cant (test.test_copy.TestCopy)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/berker/projects/cpython/master/Lib/test/test_copy.py", line 91, in test_copy_cant
    self.assertRaises(copy.Error, copy.copy, x)
  File "/home/berker/projects/cpython/master/Lib/unittest/case.py", line 743, in assertRaises
    return context.handle('assertRaises', args, kwargs)
  File "/home/berker/projects/cpython/master/Lib/unittest/case.py", line 178, in handle
    callable_obj(*args, **kwargs)
  File "/home/berker/projects/cpython/master/Lib/copy.py", line 91, in copy
    reductor = x.__reduce_ex__
  File "/home/berker/projects/cpython/master/Lib/test/test_copy.py", line 88, in __getattribute__
    raise AttributeError(name)
AttributeError: __reduce_ex__

The test was added since the initial check-in of the module, so perhaps we can this leave as-is and add a note describing the behavior?

serhiy-storchaka Jul 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Lets defer this change to a separate issue.

It changes the behavior for the case when __reduce__ is defined, but __reduce_ex__ is set to None. If this change is intentional, it is worth to the behavior of pickle (both implementations) the same change. Currently the Python implementation will call __reduce__ in this case, but the C implementation will raise a TypeError. If both __reduce__ and __reduce_ex__ are set to None, the current Python implementation will raise a PicklingError. It would be better if all implementation will have the same behavior in corner cases like this.
if reductor:
if reductor is not None:
rv = reductor(4)
else:
reductor = getattr(x, "__reduce__", None)
Expand Down Expand Up @@ -146,26 +142,22 @@ def deepcopy(x, memo=None, _nil=[]):
cls = type(x)

copier = _deepcopy_dispatch.get(cls)
if copier:
if copier is not None:
y = copier(x, memo)
else:
try:
issc = issubclass(cls, type)
except TypeError: # cls is not a class (old Boost; see SF #502085)
issc = 0
if issc:
if issubclass(cls, type):
y = _deepcopy_atomic(x, memo)
else:
copier = getattr(x, "__deepcopy__", None)
if copier:
if copier is not None:
y = copier(memo)
else:
reductor = dispatch_table.get(cls)
if reductor:
rv = reductor(x)
else:
reductor = getattr(x, "__reduce_ex__", None)
if reductor:
if reductor is not None:
rv = reductor(4)
else:
reductor = getattr(x, "__reduce__", None)
Expand Down Expand Up @@ -198,10 +190,7 @@ def _deepcopy_atomic(x, memo):
d[complex] = _deepcopy_atomic
d[bytes] = _deepcopy_atomic
d[str] = _deepcopy_atomic
try:
d[types.CodeType] = _deepcopy_atomic
except AttributeError:
pass
d[types.CodeType] = _deepcopy_atomic
d[type] = _deepcopy_atomic
d[types.BuiltinFunctionType] = _deepcopy_atomic
d[types.FunctionType] = _deepcopy_atomic
Expand Down
6 changes: 1 addition & 5 deletions Lib/pickle.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -510,11 +510,7 @@ def save(self, obj, save_persistent_id=True):
rv = reduce(obj)
else:
# Check for a class with a custom metaclass; treat as regular class
try:
issc = issubclass(t, type)
except TypeError: # t is not a class (old Boost; see SF #502085)
issc = False
if issc:
if issubclass(t, type):

serhiy-storchaka Feb 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is similar code in Python implementation of pickle. It should be synchronized.

berkerpeksag Jun 14, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Did you mean another module or is there another Python implementation of the pickle module? (Or GitHub shows your comment in a wrong place?) I've changed both try...except TypeError snippets in Lib/copy.py and Lib/pickle.py.

serhiy-storchaka Jul 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Seems I thought that we still in copy.py.
self.save_global(obj)
return

Expand Down
Back | FazBrowse Home | New Git URL