Skip to content

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
wants to merge 1 commit into from
+10 −25
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 9 additions & 20 deletions Lib/copy.py
View file
Open in desktop
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:
Copy link
Member

Choose a reason for hiding this comment

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

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

But the change LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@serhiy-storchaka serhiy-storchaka Jul 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

__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__.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
View file
Open in desktop
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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Seems I thought that we still in copy.py.

self.save_global(obj)
return

Expand Down