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.

Sign up for GitHub

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

Jump to bottom

Get rid of PyCFunction_New macro #59627

Closed
asvetlov opened this issue Jul 22, 2012 · 14 comments
Closed

Get rid of PyCFunction_New macro #59627

asvetlov opened this issue Jul 22, 2012 · 14 comments
Assignees
@asvetlov
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@asvetlov
Copy link
Contributor

BPO 15422
Nosy @loewis, @birkenfeld, @pitrou, @asvetlov
Files
  • issue15422.diff
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/asvetlov'
closed_at = <Date 2012-12-26.20:55:22.871>
created_at = <Date 2012-07-22.14:33:02.910>
labels = ['interpreter-core', 'type-feature']
title = 'Get rid of PyCFunction_New macro'
updated_at = <Date 2014-04-26.13:00:47.782>
user = 'https://github.com/asvetlov'

    bugs.python.org fields:

    activity = <Date 2014-04-26.13:00:47.782>
actor = 'pitrou'
assignee = 'asvetlov'
closed = True
closed_date = <Date 2012-12-26.20:55:22.871>
closer = 'asvetlov'
components = ['Interpreter Core']
creation = <Date 2012-07-22.14:33:02.910>
creator = 'asvetlov'
dependencies = []
files = ['27453']
hgrepos = []
issue_num = 15422
keywords = ['patch']
message_count = 14.0
messages = ['166138', '172196', '178116', '178117', '178132', '178134', '178139', '178157', '178158', '178252', '178254', '178256', '202646', '217193']
nosy_count = 5.0
nosy_names = ['loewis', 'georg.brandl', 'pitrou', 'asvetlov', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue15422'
versions = ['Python 3.4']
    @asvetlov
    Copy link
    Contributor Author

    For now (3.3 beta) PyCFunction_New defined as macro calling PyCFunction_NewEx.
    To be compatible with PEP-384 (Defining a Stable ABI) Objects/methodobject.c has trampoline function named PyCFunction_New which calls PyCFunction_NewEx.
    This is only single usage of this idiom in CPython code.
    For sake of uniformity we need to:

    @asvetlov asvetlov self-assigned this Jul 22, 2012
    @asvetlov asvetlov added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Jul 22, 2012
    @asvetlov
    Copy link
    Contributor Author

    asvetlov commented Oct 6, 2012

    Attached patch for the issue.
    BTW PyCFunction_New/PyCFunction_NewEx are part of Stable ABI but never mentioned in the documentation.

    @asvetlov
    Copy link
    Contributor Author

    bpo-16776 created for documenting PyCFunction_New/PyCFunction_NewEx

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 25, 2012

    New changeset 3a86a3f1d89a by Andrew Svetlov in branch 'default':
    Issue bpo-15422: get rid of PyCFunction_New macro
    http://hg.python.org/cpython/rev/3a86a3f1d89a

    @birkenfeld
    Copy link
    Member

    I don't think this is the only use of this particular idiom; I recall it is used every time we "amend" a function with an _Ex version.

    Why was this change necessary?

    @birkenfeld
    Copy link
    Member

    BTW it would be good if you could have at least one other developer look at issues like this and get a "LGTM" vote before committing all by yourself.

    @asvetlov
    Copy link
    Contributor Author

    1. Yes, you right. We use this idiom also for PyAST_CompileEx, PyErr_WarnEx and bunch of functions in ./Include/pythonrun.h
    2. Patch is very simple and was available for review almost 3 months.
      I assumed that developers looked on this and had no objections.
      Sorry if I was wrong.
    3. The change is not required. But, I think, it can be helpful to use direct function calls instead of macros, especially for functions which are part of Stable API.

    @birkenfeld
    Copy link
    Member

    There is no silent acceptance. No comment means that nobody reviewed it, which is no surprise given the number of open issues :)

    @birkenfeld
    Copy link
    Member

    So given #1 and #3, I would recommend reverting the part of the patch that removes the macro. Changing caller sites in CPython sources is fine.

    @asvetlov asvetlov reopened this Dec 25, 2012
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 26, 2012

    New changeset 6a56eaa5e5fb by Andrew Svetlov in branch 'default':
    Revert back PyCFunction_New macro. Keep PyCFunction_NewEx usage in python core modules (bpo-15422)
    http://hg.python.org/cpython/rev/6a56eaa5e5fb

    @asvetlov
    Copy link
    Contributor Author

    Georg, I've followed your instructions.
    Close the issue again.
    Thanks for mentorship.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 26, 2012

    New changeset 70ea05f762a1 by Andrew Svetlov in branch 'default':
    Fix compilation error for bpo-15422
    http://hg.python.org/cpython/rev/70ea05f762a1

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 11, 2013

    New changeset 267ad2ed4138 by Andrew Kuchling in branch 'default':
    bpo-15422: remove NEWS item for a change that was later reverted
    http://hg.python.org/cpython/rev/267ad2ed4138

    @pitrou
    Copy link
    Member

    pitrou commented Apr 26, 2014

    Regression in issue bpo-21354.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @asvetlov asvetlov

    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @birkenfeld @asvetlov @pitrou