Skip to content
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

Add Include/call.h file #7909

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@jdemeyer
Copy link
Contributor

commented Jun 25, 2018

This is purely moving code, no functionality or implementation is changed.

In #12, a new file Objects/call.c was added and most code implementing function calls was moved there. However, the include files were not changed. The declarations for the functions implemented in Objects/call.c are in many different places. It seems like a good idea to make this consistent and put these all together in Include/call.h instead.

This will make it easier to work on PEP 580 and friends, as well as turning METH_FASTCALL into a public interface.

CC @ncoghlan @methane @vstinner

@jdemeyer jdemeyer requested a review from python/windows-team as a code owner Jun 25, 2018

@methane

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

I don't think this type of refactoring is worth enough, but OK if really make PEP 580 easier.
But I prefer Include/internal/calls.h for private APIs, excluding semi-public APIs which are used by Cython or other 3rd parties from several years ago.

@jdemeyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

excluding semi-public APIs which are used by Cython or other 3rd parties from several years ago.

And how am I supposed to know which semi-public APIs are used by other 3rd parties? I think it's safest to put everything in a public header file, unless you have specific suggestions on what should really be considered private.

@methane

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

And how am I supposed to know which semi-public APIs are used by other 3rd parties? I think it's safest to put everything in a public header file, unless you have specific suggestions on what should really be considered private.

Your suggestion make all private APIs meaningless.
If you don't know any semi-public APIs, put all private APIs into internal.

Of course, some public APIs are implemented as macro, and private APIs used by the macros should be exposed too.

@methane

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

And METH_FASTCALL is semi-public API too :)

@methane

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

In #12, a new file Objects/call.c was added and most code implementing function calls was moved there. However, the include files were not changed. The declarations for the functions implemented in Objects/call.c are in many different places. It seems like a good idea to make this consistent and put these all together in Include/call.h instead.

At least, I can't agree here. "Implemented in which file" is implementation detail.
No need to move definition around only for keeping 1:1 pair with .c files.

So, unless "This will make it easier to work on PEP 580 and friends, " is described more concretely, I'm -1 on this.

@jdemeyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

No need to move definition around only for keeping 1:1 pair with .c files.

Why do you insist so much on there being a need to do something? Of course, there is no need for this PR. Sure, it doesn't fix any bugs.

But why should it be refused on that basis? Mild refactoring and clean up which makes code easier to read should be considered a good thing.

The reason I opened this PR is that, while working on PEP 580, I personally found it not so easy to find out what was declared where. I also didn't find a good place to add new declarations that I wanted to add to implement PEP 580.

@methane

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

But why should it be refused on that basis?

Moving code around makes:

  • hard to track history.
  • hard to backport patches to old branches.

Mild refactoring and clean up which makes code easier to read should be considered a good thing.

I don't feel this refactoring is really makes code easier to read.
For example, I feel methodobject.h is better place for PyMethodDef than call.h.

I even think methodobject.h is better place for _PyMethodDef_RawFastCallDict definition too.
Implementation of the function went to call.c for some practical reason.
But I think no need to move definition to worse place too.

@vstinner
Copy link
Member

left a comment

I'm not convinced that this PR makes the code easier to maintain: https://mail.python.org/pipermail/python-dev/2018-June/154088.html

@bedevere-bot

This comment has been minimized.

Copy link

commented Jun 26, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@jdemeyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

@vstinner: can you clarify what you mean with "awaiting changes"? My impression from reading python-dev is that this PR is going to be rejected anyway.

@vstinner

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

@vstinner: can you clarify what you mean with "awaiting changes"?

I suggest to reject this PR. I clicked on "Request change" to "vote -1".

@jdemeyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

Thanks for the clarification (it was not clear to me whether your reaction was neutral or negative). I'll close it when the python-dev discussion dies out. In particular, I would like the opinion of @ncoghlan

jdemeyer added a commit to jdemeyer/cpython that referenced this pull request Jun 26, 2018

@jdemeyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

I don't really know what to make of Nick Coghlan's post on python-dev, but I guess nobody will mind closing and burying this PR.

@jdemeyer jdemeyer closed this Jun 28, 2018

@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2018

The short version of my python-dev post is that I think the proposed abstract.h -> call.h function prototype extraction is a decent idea, but I don't like the proposed changes to the API headers for the concrete object definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.