| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
I don't think this type of refactoring is worth enough, but OK if really make PEP 580 easier. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Your suggestion make all private APIs meaningless. Of course, some public APIs are implemented as macro, and private APIs used by the macros should be exposed too. |
Sorry, something went wrong.
|
And METH_FASTCALL is semi-public API too :) |
Sorry, something went wrong.
At least, I can't agree here. "Implemented in which file" is implementation detail. So, unless "This will make it easier to work on PEP 580 and friends, " is described more concretely, I'm -1 on this. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Moving code around makes:
I don't feel this refactoring is really makes code easier to read. I even think methodobject.h is better place for _PyMethodDef_RawFastCallDict definition too. |
Sorry, something went wrong.
There was a problem hiding this 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
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
I suggest to reject this PR. I clicked on "Request change" to "vote -1". |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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