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

berkerpeksag · 2017-02-21T06:07:47Z

When doing getattr lookups with a default of "None", it
now uses an "is" comparison against None which is both
faster and more correct
Used getattr() instead of hasattr()
Removed obsolete branches where it's impossible to be
called in Python 3

Patch by Brandon Rhodes.

https://bugs.python.org/issue11572

berkerpeksag · 2017-02-21T06:12:39Z

Another possible cleanup opportunity (added in https://hg.python.org/cpython/rev/c4715c9f442f)

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

serhiy-storchaka · 2017-02-21T08:36:40Z

Lib/copy.py

@@ -217,6 +203,8 @@ def _deepcopy_list(x, memo, deepcopy=deepcopy):
 d[list] = _deepcopy_list

 def _deepcopy_tuple(x, memo, deepcopy=deepcopy):
+    if not x:


Empty tuple is a singleton. The first deepcopied empty tuple will be memoized, all other occurrences will be retrieved from the memo dict. I amn't sure that this change adds performance gain rather than regression.

Ok, removed the "optimization" :)

serhiy-storchaka · 2017-02-21T08:46:27Z

Lib/copy.py

@@ -278,8 +266,9 @@ def _reconstruct(x, memo, func, args,
    if state is not None:
        if deep:
            state = deepcopy(state, memo)
-        if hasattr(y, '__setstate__'):
-            y.__setstate__(state)
+        setstate = getattr(y, '__setstate__', None)


This is a new feature. Setting __setstate__ to None triggers the default behavior. It should be documented in Misc/NEWS. I don't know whether it is worth to document it in the module documentation and What's New. Python implementation of pickle does the same, but C implementation doesn't. If change C implementation of pickle, this should be documented as new feature.

See also related bpo-26579 about other way to call default __setstate__ implementation.

Added a test for this behavior: test_copy_setstate_none

TODOs:

Document this behavior in the NEWS entry.

I'm not sure this is a desired behavior. If it is, the same behavior should have both pickle implementations. I suggest to defer this change to a separate issue.

serhiy-storchaka · 2017-02-21T08:56:33Z

Lib/pickle.py

-            except TypeError: # t is not a class (old Boost; see SF #502085)
-                issc = False
-            if issc:
+            if issubclass(t, type):


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

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.

Seems I thought that we still in copy.py.

serhiy-storchaka · 2017-02-21T09:19:20Z

Lib/copy.py

        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)


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

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?

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.

serhiy-storchaka · 2017-02-21T09:24:37Z

Lib/copy.py

        rv = reductor(x)
    else:
        reductor = getattr(x, "__reduce_ex__", None)
-        if reductor:
+        if reductor is not None:
+            # note that object.__reduce_ex__ calls __reduce__


There is small behavior change. Currently if set __reduce_ex__ to None, copy() falls back to __reduce__. Patched code raises Error.

This change looks not bad to me.

Added a test for this behavior.

serhiy-storchaka · 2017-02-21T09:27:27Z

Lib/copy.py

@@ -84,22 +84,19 @@ def copy(x):
        return _copy_immutable(x)

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


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

But the change LGTM.

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

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

brettcannon · 2018-02-21T01:51:42Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

berkerpeksag · 2018-06-14T00:47:18Z

Thanks for the review, Serhiy. I've addressed all of your review comments except the ones about x.__reduce_ex__.

Also, I still need to a NEWS entry.

berkerpeksag · 2018-07-09T14:06:35Z

@serhiy-storchaka can you take a look at the latest version of this PR?

serhiy-storchaka

The PR contains not just cleanup, but leads to some user-visible behavior changes. They are not consistent with pickle implementations, and it is not obvious that they are purposed. I suggest to revert these changes, and open a separate issue for discussing about the meaning of setting None to special methods like __reduce_ex__, __getstate__ and __setstate__.

serhiy-storchaka · 2018-07-09T16:18:55Z

Lib/copy.py

@@ -84,22 +84,19 @@ def copy(x):
        return _copy_immutable(x)

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


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

serhiy-storchaka · 2018-07-09T16:28:10Z

Lib/pickle.py

-            except TypeError: # t is not a class (old Boost; see SF #502085)
-                issc = False
-            if issc:
+            if issubclass(t, type):


Seems I thought that we still in copy.py.

serhiy-storchaka · 2018-07-09T17:36:47Z

Lib/copy.py

        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)


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.

serhiy-storchaka · 2018-07-09T17:39:21Z

Lib/copy.py

@@ -278,8 +266,9 @@ def _reconstruct(x, memo, func, args,
    if state is not None:
        if deep:
            state = deepcopy(state, memo)
-        if hasattr(y, '__setstate__'):
-            y.__setstate__(state)
+        setstate = getattr(y, '__setstate__', None)


I'm not sure this is a desired behavior. If it is, the same behavior should have both pickle implementations. I suggest to defer this change to a separate issue.

serhiy-storchaka · 2018-07-09T17:40:58Z

Lib/copy.py

-                        else:
-                            raise Error(
-                                "un(deep)copyable object of type %s" % cls)
+                        raise Error( "un(deep)copyable object of type %s" % cls)


The same as for copy(). I suggest to defer reduction of the __reduce__ branch to a separate issue.

berkerpeksag · 2018-07-09T18:00:36Z

@serhiy-storchaka thanks for the review! Reverted them in 3b49327. Do you think we need a NEWS entry after 3b49327?

serhiy-storchaka · 2018-07-09T19:06:04Z

Thank you @berkerpeksag. I have no opinion about the NEWS entry.

berkerpeksag · 2018-07-09T19:24:53Z

Ok, I'm not going to add a NEWS entry since it's just a cleanup PR.

* When doing getattr lookups with a default of "None", it now uses an "is" comparison against None which is more correct * Removed outdated code Patch by Brandon Rhodes.

Addressed Serhiy's review comments.

berkerpeksag · 2018-07-09T19:45:17Z

@serhiy-storchaka do you know what the problem with the build? I can't reproduce it locally.

Fixing C file whitespace ... 1 file:
  Modules/unicodedata_db.h
Fixing docs whitespace ... 0 files
Please fix the 1 file(s) with whitespace issues

It looks like something is wrong with the patchcheck script.

cframeobject.h and channelobject.h are now empty and obsolete. They will be removed by the next commit.

Move the headers to Include and Include/internal, fix the #include directives and update Makefile.in.pre and PCbuild-files.

…ols/msi Remove the special handling of Stackless include files from the msi-installer.

Remove the Stackless source dir from the include-path.

Remove the Stackless specific include-path.

…c headers Move the definition of the macro SLP_SEH32 to stackless_struct.h

Names of "public" header files use prefix "stackless", all other use "slp". This is consistent with symbol names.

berkerpeksag added the type-feature A feature request or enhancement label Feb 21, 2017

berkerpeksag requested a review from serhiy-storchaka February 21, 2017 06:07

the-knights-who-say-ni added the CLA signed label Feb 21, 2017

serhiy-storchaka previously requested changes Feb 21, 2017

View reviewed changes

brettcannon added the awaiting changes label Feb 21, 2018

berkerpeksag force-pushed the 11572-copy-improvements branch from 569fdda to 90d74b3 Compare June 14, 2018 00:44

serhiy-storchaka reviewed Jul 9, 2018

View reviewed changes

berkerpeksag force-pushed the 11572-copy-improvements branch from 90d74b3 to 3b49327 Compare July 9, 2018 17:59

berkerpeksag added the skip news label Jul 9, 2018

bpo-11572: Make several minor improvements to copy module

0a88fde

* When doing getattr lookups with a default of "None", it now uses an "is" comparison against None which is more correct * Removed outdated code Patch by Brandon Rhodes.

berkerpeksag force-pushed the 11572-copy-improvements branch from 3b49327 to 0a88fde Compare July 9, 2018 19:31

berkerpeksag closed this Jul 9, 2018

berkerpeksag deleted the 11572-copy-improvements branch July 9, 2018 20:14

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 16, 2019

Stackless issue python#207: Empty obsolete header files

a756c79

cframeobject.h and channelobject.h are now empty and obsolete. They will be removed by the next commit.

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 16, 2019

Stackless issue python#207: Move/rename/remove Stackless header files

2d7c76c

Move the headers to Include and Include/internal, fix the #include directives and update Makefile.in.pre and PCbuild-files.

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 16, 2019

Stackless issue python#207: Remove the Stackless include dirs from To…

3f48801

…ols/msi Remove the special handling of Stackless include files from the msi-installer.

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 16, 2019

Stackless issue python#207: Update the Mac Makefile.in

46106ce

Remove the Stackless source dir from the include-path.

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 16, 2019

Stackless issue python#207: Update distutils

71bb55a

Remove the Stackless specific include-path.

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 18, 2019

Stackless issue python#207: Minor fix of PCbuild files

45af12e

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 18, 2019

Stackless issue python#207: don't include internal headers from publi…

2eee330

…c headers Move the definition of the macro SLP_SEH32 to stackless_struct.h

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 18, 2019

Stackless issue python#207: consistent header files names

90b1e00

Names of "public" header files use prefix "stackless", all other use "slp". This is consistent with symbol names.

akruis pushed a commit to akruis/cpython that referenced this pull request Jan 18, 2019

Stackless issue python#207: add changelog entry

b06944e

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Update pytest from 4.2.0 to 4.2.1 (#207)

fd02e8b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

berkerpeksag commented Feb 21, 2017 •

edited by bedevere-bot

Loading

berkerpeksag commented Feb 21, 2017

serhiy-storchaka Feb 21, 2017

berkerpeksag Jun 14, 2018

serhiy-storchaka Feb 21, 2017

berkerpeksag Jun 14, 2018 •

edited by serhiy-storchaka

Loading

serhiy-storchaka Jul 9, 2018

serhiy-storchaka Feb 21, 2017

berkerpeksag Jun 14, 2018

serhiy-storchaka Jul 9, 2018

serhiy-storchaka Feb 21, 2017

berkerpeksag Jun 14, 2018

serhiy-storchaka Jul 9, 2018

serhiy-storchaka Feb 21, 2017

berkerpeksag Jun 14, 2018

serhiy-storchaka Feb 21, 2017

berkerpeksag Jun 14, 2018

serhiy-storchaka Jul 9, 2018 •

edited

Loading

brettcannon commented Feb 21, 2018

berkerpeksag commented Jun 14, 2018

berkerpeksag commented Jul 9, 2018

serhiy-storchaka left a comment

serhiy-storchaka Jul 9, 2018 •

edited

Loading

serhiy-storchaka Jul 9, 2018

serhiy-storchaka Jul 9, 2018

serhiy-storchaka Jul 9, 2018

serhiy-storchaka Jul 9, 2018

berkerpeksag commented Jul 9, 2018

serhiy-storchaka commented Jul 9, 2018

berkerpeksag commented Jul 9, 2018

berkerpeksag commented Jul 9, 2018

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

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

Conversation

berkerpeksag commented Feb 21, 2017 • edited by bedevere-bot Loading

berkerpeksag commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag Jun 14, 2018 • edited by serhiy-storchaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

brettcannon commented Feb 21, 2018

berkerpeksag commented Jun 14, 2018

berkerpeksag commented Jul 9, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Jul 9, 2018

serhiy-storchaka commented Jul 9, 2018

berkerpeksag commented Jul 9, 2018

berkerpeksag commented Jul 9, 2018

berkerpeksag commented Feb 21, 2017 •

edited by bedevere-bot

Loading

berkerpeksag Jun 14, 2018 •

edited by serhiy-storchaka

Loading

serhiy-storchaka Jul 9, 2018 •

edited

Loading

serhiy-storchaka Jul 9, 2018 •

edited

Loading