-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[mypyc] feat: extend get_expr_length for enumerate, map, zip, range, list, tuple, sorted, and reversed CallExpr [3/4]
#19927
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
base: master
Are you sure you want to change the base?
Conversation
This PR is pretty simple, I just extended get_expr_length to work for a few more obvious cases: - `builtins.enumerate` - `builtins.map` - `builtins.zip` - `builtins.range` This PR is ready for review. Are you going to want tests for all of these? I don't want to spend time now until I know for sure.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
get_expr_length for enumerate, map, zip, range, list, tuple, sorted, and reversed CallExpr
|
I added an IR test that covers a few of these, and I learned we need to merge #19935 before we can use it how I intended for the test. For now I'm leaving the test in just so it can be compared to #19935 The IR in #19935 is much cleaner, you'll see a r1 = PyTuple_New(5) which shows that the code from the PR did its job. |
for more information, see https://pre-commit.ci
get_expr_length for enumerate, map, zip, range, list, tuple, sorted, and reversed CallExprget_expr_length for enumerate, map, zip, range, list, tuple, sorted, and reversed CallExpr [3/4]
| return not val | ||
|
|
||
| def test() -> None: | ||
| # this tuple is created from a very complex genexp but we can still compute the length and preallocate the tuple |
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 can't see where the preallocation happens in the test case output. It seems to create a list and then convert it to a list?
@JukkaL FYI wrt the IR Easiest path forward is probably merging this one first, given that #19935 proves that the specific change to our length logic IS successful, even though it doesn't show on the current IR test, and then letting me rebase #19935 on top of master which will correct the IR test from this PR when we merge it. |
This PR is pretty simple, I just extended get_expr_length to work for a few more obvious cases:
builtins.enumeratebuiltins.mapbuiltins.zipbuiltins.rangebuiltins.listbuiltins.tuplebuiltins.sortedbuiltins.reversedThis PR is ready for review. Are you going to want tests for all of these? I didn't want to spend time now until I know for sure.
All of the
get_expr_lengthPRs are entirely independent and can be reviewed/merged in any order.