Skip to content

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn commented Jan 7, 2023

See the original issue for microbenchmarks.

Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

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

This PR results in a behaviour change for the following edge case.

On main:

>>> s =slice([1, 2], [3, 4], [5, 6]) >>> from copy import deepcopy >>> t = deepcopy(s) >>> t.start is s.start False

With this PR:

>>> s =slice([1, 2], [3, 4], [5, 6]) >>> from copy import deepcopy >>> t = deepcopy(s) >>> t.start is s.start True

@sobolevn
Copy link
MemberAuthor

Indeed, I haven't thought about this case. One of my main assumptions was that slice cannot contain nested structures. Because I've only seen int samples in real life. But, if lists are valid and used - this should not be merged. Because deepcopy does not work as promised: it is a shallow copy now.

@sobolevnsobolevn closed this Jan 7, 2023
@AlexWaygood
Copy link
Member

Yeah, it's a really obscure edge case, but I can find some examples in the wild: https://grep.app/search?q=%20slice%28%5B&case=true&words=true&filter[lang][0]=Python.

Would you maybe like to add a test for this edge case? I very nearly merged the PR on the basis that all the tests passed, and it seemed like an impressive optimisation :)

miss-islington pushed a commit that referenced this pull request Jan 11, 2023
CC @AlexWaygood as the reviewer of #100818 Automerge-Triggered-By: GH:AlexWaygood
sobolevn added a commit to sobolevn/cpython that referenced this pull request Jan 12, 2023
CC @AlexWaygood as the reviewer of python#100818 Automerge-Triggered-By: GH:AlexWaygood. (cherry picked from commit 729ab9b) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
sobolevn added a commit to sobolevn/cpython that referenced this pull request Jan 12, 2023
CC @AlexWaygood as the reviewer of python#100818 Automerge-Triggered-By: GH:AlexWaygood. (cherry picked from commit 729ab9b) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@sobolevn@AlexWaygood@bedevere-bot