This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Fold constant slicing with slices
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.9
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: BTaskaya, eric.smith, pablogsal, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-01-05 15:51 by BTaskaya, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 17838 closed BTaskaya, 2020-01-05 15:57
Messages (14)
msg359350 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-01-05 15:51
>>> def g(): "abcde"[2:4]
... 
>>> g.__code__.co_consts
(None, 'abcde', 2, 4)

to 

>>> def g(): "abcde"[2:4]
... 
>>> g.__code__.co_consts
(None, 'cd')

(I have a patch)
msg359352 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2020-01-05 16:34
Does this occur often enough that it's worth the added code?
msg359358 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-01-05 17:16
I am with Eric. Why would anyone write:


"abcde"[2:4]

Instead of just

cd

?
msg359359 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-01-05 17:17
Specially because you need to do the mental calculation to know what the indexes are for the letters you want, and at that point you better write that string anyways, no?
msg359361 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-01-05 17:20
We already have a folding operation for Index access ("abcde"[2]) and there was a todo (left by @methane I guess) about supporting other slicings. I think this would look inconsistent if one is support and not the other one.
msg359364 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2020-01-05 17:52
Then I'd suggest removing the optimization for "abcde"[2], too. I've never seen this in real code, and I can't come up with a scenario where it would be performance critical. Remember, one of the goals for CPython is to be reasonable readable, not maximally tricky.

Could you point to the code (or ideally the commit) with methane's comment?

I'm not trying to be a jerk, though. I do appreciate your patch, I just think it's not appropriate for CPython.
msg359365 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-01-05 17:56
> Then I'd suggest removing the optimization for "abcde"[2], too. I've never seen this in real code, and I can't come up with a scenario where it would be performance critical. Remember, one of the goals for CPython is to be reasonable readable, not maximally tricky.

I dont think this patch will decrease readability and make code complex.

> Could you point to the code (or ideally the commit) with methane's comment?
https://github.com/python/cpython/blob/master/Python/ast_opt.c#L319 (PR 2858)
msg359366 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-01-05 18:08
I was reading the pull request and I have to say that I find using a switch instead of a very crowded "if" ward a bit more readable.

I would be +0 on this case.
msg359367 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-01-05 18:08
I've scanned stdlib, django and flask and no usage so far. There is one in the test (which basically verifies behavior of slice) and one in the argument clinic which passes the newline for a multiline string. 

cpython/Tools/clinic/clinic.py:1185
cpython/Lib/test/test_descr.py:3789
msg359368 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-01-05 18:18
The one in cpython/Tools/clinic/clinic.py:1185 is sort of an anti-pattern as it should be using 

'''\
Something something
'''

instead of skipping the newline.
msg359370 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python committer) Date: 2020-01-05 18:33
It looks like we have some usage for normal indexing (not this PR, the original optimization);

cpython/cpython/Lib/base64.py:382
cpython/cpython/Lib/base64.py:382
cpython/cpython/Lib/base64.py:393
cpython/cpython/Lib/base64.py:397
cpython/cpython/Lib/pickle.py:306
cpython/cpython/Lib/pickle.py:1291
cpython/cpython/Lib/glob.py:155
cpython/cpython/Lib/test/test_mmap.py:59
cpython/cpython/Lib/test/test_mmap.py:63
cpython/cpython/Lib/test/test_mmap.py:574
cpython/cpython/Lib/test/test_codecs.py:2317
cpython/cpython/Lib/test/test_codecs.py:2323
cpython/cpython/Lib/test/test_contextlib.py:1025
cpython/cpython/Lib/test/test_struct.py:645
msg359380 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-05 20:16
The optimization for constant indexes is handy for things like b'A'[0]. I do not know cases for constant slices of constant strings.

Even if there are cases, we need to prove that they are common enough. The compiler does not perform all possible constant foldings, it does it only to support common cases like 24*60*60 or 2**32-1.
msg359382 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-01-05 20:51
> Even if there are cases, we need to prove that they are common enough

I don't think cases for an extended slicing are common enough. In addition to stdlib, Django and flask (checked by Batuhan) I checked many popular 3rd party packages and there is not an occurrence of the pattern.
msg359383 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-01-05 21:30
Then I suggest to focus on optimizing real code.
History
Date User Action Args
2022-04-11 14:59:24adminsetgithub: 83404
2020-01-05 21:30:52serhiy.storchakasetstatus: open -> closed
resolution: rejected
messages: + msg359383

stage: patch review -> resolved
2020-01-05 20:51:58pablogsalsetmessages: + msg359382
2020-01-05 20:16:18serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg359380
2020-01-05 18:33:27BTaskayasetmessages: + msg359370
2020-01-05 18:18:31pablogsalsetmessages: + msg359368
2020-01-05 18:08:29BTaskayasetmessages: + msg359367
2020-01-05 18:08:21pablogsalsetmessages: + msg359366
2020-01-05 17:56:25BTaskayasetmessages: + msg359365
2020-01-05 17:52:31eric.smithsetmessages: + msg359364
2020-01-05 17:20:45BTaskayasetmessages: + msg359361
2020-01-05 17:17:40pablogsalsetmessages: + msg359359
2020-01-05 17:16:36pablogsalsetnosy: + pablogsal
messages: + msg359358
2020-01-05 16:34:19eric.smithsetnosy: + eric.smith
messages: + msg359352
2020-01-05 15:57:02BTaskayasetkeywords: + patch
stage: patch review
pull_requests: + pull_request17265
2020-01-05 15:51:54BTaskayacreate