GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add Include/call.h file by jdemeyer · Pull Request #7909 · python/cpython

Conversation

@jdemeyer

@jdemeyer jdemeyer commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jdemeyer
Add Include/call.h
5e42cd3
@jdemeyer
jdemeyer requested a review from a team as a code owner June 25, 2018 13:05
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 25, 2018
@bedevere-bot bedevere-bot added the awaiting review label Jun 25, 2018
@jdemeyer jdemeyer mentioned this pull request Jun 25, 2018
bpo-29524: Add Objects/call.c file #12
Merged
@methane

methane commented Jun 26, 2018

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer

jdemeyer commented Jun 26, 2018

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@methane

methane commented Jun 26, 2018

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@methane

methane commented Jun 26, 2018

Copy link
Copy Markdown
Member

And METH_FASTCALL is semi-public API too :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@methane

methane commented Jun 26, 2018

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer

jdemeyer commented Jun 26, 2018

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@methane

methane commented Jun 26, 2018

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner requested changes Jun 26, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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'm not convinced that this PR makes the code easier to maintain: https://mail.python.org/pipermail/python-dev/2018-June/154088.html

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Jun 26, 2018
@bedevere-bot

bedevere-bot commented Jun 26, 2018

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer

jdemeyer commented Jun 26, 2018

Copy link
Copy Markdown
Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jun 26, 2018

Copy link
Copy Markdown
Member

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer

jdemeyer commented Jun 26, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer

jdemeyer commented Jun 28, 2018

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jdemeyer jdemeyer closed this Jun 28, 2018
@ncoghlan

ncoghlan commented Jun 30, 2018

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jdemeyer @methane @bedevere-bot @vstinner @ncoghlan @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.