Skip to content

Conversation

TH3CHARLie
Copy link
Collaborator

@TH3CHARLie TH3CHARLie commented May 27, 2020

relates mypyc/mypyc#709

This PR supports top-level function ops via recently added CallC IR. To demonstrate the idea, it transform to_list op from PrimitiveOp to CallC. It also refines CallC with arguments coercing and support of steals.

@TH3CHARLie
Copy link
Collaborator Author

Matching func_ops(where to_list op originally resides) happens in two places: ll_builder/builtin_call and builder/call_refexpr_with_args. I changed them accordingly, but I also observe that for to_list case(at least for simple cases I tested), only the latter one is doing the work.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks great!

@JukkaL
Copy link
Collaborator

JukkaL commented May 28, 2020

Let me know when you are ready with the PR (it now says WIP).

@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented May 28, 2020

Let me know when you are ready with the PR (it now says WIP).

Yes, I'd say it's WIP because I intend to include list_set_item_op as well and there are some other things that may need to discuss.

@TH3CHARLie TH3CHARLie changed the title [WIP][mypyc] Support top level function ops via CallC [mypyc] Support top level function ops via CallC May 28, 2020
@TH3CHARLie
Copy link
Collaborator Author

TH3CHARLie commented May 28, 2020

I think it's now ready for review(assuming all tests passed).

Updates(57a1b91): Well, many tests failed. I observe and summarize that are two kinds of failing tests. The first one is IR tests, which can be fixed quickly. The second kind is about traceback, changing list_set_item op somehow produces much less traceback info.

updates(983d635): I revert list_set_item_op as I think it is too intrusive to test suite. Supporting it requires changing a lot of tests and make the PR pretty huge. And figuring out why changing it from PrimitiveOp to CallC causes different traceback behavior needs time and discussion.

@TH3CHARLie TH3CHARLie requested a review from JukkaL May 28, 2020 15:22
@@ -83,7 +84,6 @@ def emit_new(emitter: EmitterInterface, args: List[str], dest: str) -> None:
error_kind=ERR_FALSE,
emit=call_emit('CPyList_SetItem'))


Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I delete a blank line here and I find that one/two blank line(s) is used interchangeably. I am surprised that flake8 is OK with this

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Left some comments about coercing the result type.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Looks great.

@JukkaL JukkaL merged commit 8457e50 into python:master Jun 1, 2020
@TH3CHARLie
Copy link
Collaborator Author

Thanks for the review!

@TH3CHARLie TH3CHARLie deleted the call-c-to-list branch June 1, 2020 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants