◐ Shell
reader mode source ↗
Skip to content

bpo-31938: Convert selectmodule.c to Argument Clinic#4265

Merged
taleinat merged 19 commits into
python:masterfrom
taleinat:bpo-31938_AC_selectmodule
Jun 30, 2018
Merged

bpo-31938: Convert selectmodule.c to Argument Clinic#4265
taleinat merged 19 commits into
python:masterfrom
taleinat:bpo-31938_AC_selectmodule

Conversation

@taleinat

@taleinat taleinat commented Nov 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is a complete Argument Clinic conversion of Modules/selectmodule.c.

The initial commit on this branch is the mostly fixed application of the latest patch on bpo-20182. Later changes were each done in a separate commit to facilitate review.

Minor note: Some of the early commits on the branch don't compile successfully.

https://bugs.python.org/issue31938

@taleinat
bpo-20182: initial application of issue20182.selectmodule.v2.patch …
391f243 
Not in working condition, things need to be moved around to work with
generation of AC code in a separate file.
@taleinat
bpo-20182: AC convert new select.kqueue and fix missing summary lines
3423836
@taleinat
bpo-20182: selectmodule.c AC: move type and method defintions to bottom …
778d240 
This is required to be able to include the generated AC code from
clinic/selectmodule.c.h, which must appear before these definitions
but after all typedefs and type declarations.
@taleinat
bpo-20182: selectmodule.c AC: add file descriptor argument converter …
378a27f 
The converter was copied from Modules/posixmodule.c, but changed to
check whether fd == -1 rather than fd < 0 as an error condition from
calling PyOjbect_AsFileDescriptor().

Note: AC output in Modules/clinic/selectmodule.c.h was manually
modified in this commit to get the build working. This may be due to
an AC bug.
@taleinat
bpo-20182: selectmodule.c AC: convert select.devpoll to AC …
fc964f0 
Note: AC output in Modules/clinic/selectmodule.c.h was manually
modified in this commit to get the build working. This may be due to
an AC bug.
@taleinat
bpo-20182: selectmodule.c AC: fix and standardize use of ushort_conve… …
b415d95 
…rter
@taleinat
bpo-20182: selectmodule.c AC: don't "as"-rename epoll method functions …
352004c 
Also change the doc-string of epoll.poll:
* use AC's per-argument docs to describe the arguments
* revert an addition describing the return value

Note: AC output in Modules/clinic/selectmodule.c.h was manually
modified in this commit to get the build working. This may be due to
an AC bug.
@taleinat
bpo-20182: selectmodule.c AC: fix two compilation errors in epoll
b91b2db
@taleinat
bpo-20182: selectmodule.c AC: re-run clinic.py after its recent bugfix
7939e70
vstinner
vstinner reviewed Nov 3, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Thank you for this nice PR. While I may have other comments later, and @serhiy-storchaka already added comments, I like how the new code looks like ;-)

vstinner
vstinner reviewed Jun 22, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

This PR is just giant. I would suggest to split it into multiple smaller PRs. For example, why not starting by converting only select()?

@taleinat

taleinat commented Jun 22, 2018

Copy link
Copy Markdown
Contributor Author

This PR is just giant. I would suggest to split it into multiple smaller PRs. For example, why not starting by converting only select()?

I can understand why you suggest this. I'm discouraged by the amount of work required in addition to that already invested here. I'm further discouraged since I've done exactly what you suggest for the AC conversion of itertools, beginning with converting only groupby(), but that has been completely ignored so far (PR #4170).

@vstinner

vstinner commented Jun 22, 2018

Copy link
Copy Markdown
Member

I've done exactly what you suggest for the AC conversion of itertools, beginning with converting only groupby(), but that has been completely ignored so far (PR #4170).

I like AC and want to convert everything to AC, but I have limited bandwidth to review such AC. Please don't hesitate to ping me frequently until I reviewed all of them :-)

@serhiy-storchaka

serhiy-storchaka commented Jun 22, 2018

Copy link
Copy Markdown
Member

Making all changes with a single PR LGTM. This is unavoidable when convert to AC such module. Let me to recall where we stopped last time. I guess there were just minor problems which should be resolved.

@taleinat

taleinat commented Jun 22, 2018

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka

Let me to recall where we stopped last time.

I've got a patch ready which addresses nearly all of those issues. If you're interested I'll get it done and push it to this branch. I suggest that you wait until that's done before reviewing again.

@taleinat

taleinat commented Jun 22, 2018

Copy link
Copy Markdown
Contributor Author

@vstinner

I like AC and want to convert everything to AC, but I have limited bandwidth to review such AC. Please don't hesitate to ping me frequently until I reviewed all of them :-)

I greatly appreciate your bandwidth, it's obvious you spend a lot of time and effort to address so many different issues! I realize that AC is lower priority than most things and accept that. I'll work on moving my AC patches one at a time, in hopes of getting things moving in smaller steps will require less ongoing commitment from other core devs.

@taleinat

taleinat commented Jun 22, 2018

Copy link
Copy Markdown
Contributor Author

@vstinner, AFAICT all of your comments in the last batch are regarding code and comments which I mostly moved around rather than actually wrote or edited. I'd rather make such changes after the AC conversion if you don't mind. I'd gladly collect your comments here and prepare another PR with just those improvements, after this is done with.

@taleinat

taleinat commented Jun 22, 2018

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka, I've fixed all of the outstanding issues brought up here that it was clear should be fixed. I think this is ready for another review!

I've left the epoll sizehint behavior as-is, i.e. this patch doesn't change the existing behavior, questionable though it may be. We are addressing that on the dedicated issue bpo-32568.

I've also not addressed the comments from @vstinner's latest review, since they are all about things I haven't actually changed here.

33 hidden items Load more…
@taleinat

taleinat commented Jun 25, 2018

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka, I think this is ready for another review!

I've addressed your latest two comments and fixed a couple of bugs related to select.kqueue (now all tests are passing).

@taleinat
Merge branch 'master' into bpo-31938_AC_selectmodule …
1474605 
# Conflicts:
#	Modules/selectmodule.c
@taleinat taleinat merged commit 6dc57e2 into python:master Jun 30, 2018
@bedevere-bot bedevere-bot removed the label Jun 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner

@vadmium vadmium vadmium left review comments

@serhiy-storchaka serhiy-storchaka

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@taleinat @vstinner @serhiy-storchaka @vadmium @the-knights-who-say-ni @bedevere-bot