-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
gh-126298: Don't deduplicate slice constants based on equality #126398
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.
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
Conversation
da12eed
to
af304c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a typo in one test, and some further suggestions for completeness:
Lib/test/test_compile.py
Outdated
@@ -1384,53 +1384,90 @@ def check_op_count(func, op, expected): | |||
actual += 1 | |||
self.assertEqual(actual, expected) | |||
|
|||
def check_num_consts(func, typ, expected): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think everywhere this is used it's called with 0
expected. Maybe just replace those calls with check_consts(func, typ, set())
since both functions are so similar?
Lib/test/test_compile.py
Outdated
def store(): | ||
x[a:b] = y | ||
x [a:] = y | ||
x[a:] = y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whitespace might be intentional... load
has the exact same whitespace above. We are testing the compiler, after all.
I'm inclined to just leave it.
|
||
def load(): | ||
return x[a:b] + x [a:] + x[:b] + x[:] | ||
|
||
check_op_count(load, "BINARY_SLICE", 3) | ||
check_op_count(load, "BUILD_SLICE", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_op_count(load, "BUILD_SLICE", 0) | |
check_op_count(load, "BUILD_SLICE", 0) | |
check_op_count(load, "BINARY_SUBSCR", 1) |
x[:b] = y | ||
x[:] = y | ||
|
||
check_op_count(store, "STORE_SLICE", 3) | ||
check_op_count(store, "BUILD_SLICE", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_op_count(store, "BUILD_SLICE", 0) | |
check_op_count(store, "BUILD_SLICE", 0) | |
check_op_count(store, "STORE_SUBSCR", 1) |
def long_slice(): | ||
return x[a:b:c] | ||
|
||
check_op_count(long_slice, "BUILD_SLICE", 1) | ||
check_op_count(long_slice, "BINARY_SLICE", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_op_count(long_slice, "BINARY_SLICE", 0) | |
check_op_count(long_slice, "BINARY_SLICE", 0) | |
check_op_count(long_slice, "BINARY_SUBSCR", 1) |
Lib/test/test_compile.py
Outdated
check_op_count(aug, "BINARY_SLICE", 1) | ||
check_op_count(aug, "STORE_SLICE", 1) | ||
check_op_count(aug, "BUILD_SLICE", 0) | ||
check_num_consts(long_slice, slice, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_num_consts(long_slice, slice, 0) | |
check_num_consts(aug, slice, 0) |
def aug(): | ||
x[a:b] += y | ||
|
||
check_op_count(aug, "BINARY_SLICE", 1) | ||
check_op_count(aug, "STORE_SLICE", 1) | ||
check_op_count(aug, "BUILD_SLICE", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_op_count(aug, "BUILD_SLICE", 0) | |
check_op_count(aug, "BUILD_SLICE", 0) | |
check_op_count(aug, "BINARY_SUBSCR", 0) | |
check_op_count(aug, "STORE_SUBSCR", 0) |
def aug_const(): | ||
x[1:2] += y | ||
|
||
check_op_count(aug_const, "BINARY_SLICE", 0) | ||
check_op_count(aug_const, "STORE_SLICE", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_op_count(aug_const, "STORE_SLICE", 0) | |
check_op_count(aug_const, "STORE_SLICE", 0) | |
check_op_count(aug_const, "BINARY_SUBSCR", 1) | |
check_op_count(aug_const, "STORE_SUBSCR", 1) |
check_op_count(aug, "BUILD_SLICE", 0) | ||
check_op_count(aug_const, "BINARY_SLICE", 0) | ||
check_op_count(aug_const, "STORE_SLICE", 0) | ||
check_consts(aug_const, slice, 1) | ||
check_op_count(compound_const_slice, "BINARY_SLICE", 0) | ||
check_op_count(compound_const_slice, "BUILD_SLICE", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_op_count(compound_const_slice, "BUILD_SLICE", 0) | |
check_op_count(compound_const_slice, "BUILD_SLICE", 0) | |
check_op_count(compound_const_slice, "STORE_SLICE", 0) | |
check_op_count(compound_const_slice, "BINARY_SUBSCR", 1) |
@@ -1385,52 +1385,90 @@ def check_op_count(func, op, expected): | |||
self.assertEqual(actual, expected) | |||
|
|||
def check_consts(func, typ, expected): | |||
slice_consts = 0 | |||
all_consts = set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is working correctly - slices just defer to their contents for hash/equality, so this will be deduplicating the slices in the same way this is trying to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, nice catch!
…ython#126398) * pythongh-126298: Don't deduplicated slice constants based on equality * NULL check for PySlice_New * Fix refcounting * Fix refcounting some more * Fix refcounting * Make tests more complete * Fix tests
…ython#126398) * pythongh-126298: Don't deduplicated slice constants based on equality * NULL check for PySlice_New * Fix refcounting * Fix refcounting some more * Fix refcounting * Make tests more complete * Fix tests
Uh oh!
There was an error while loading. Please reload this page.