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

PyList_APPEND (append without incref) #50865

Closed
ideasman42 mannequin opened this issue Aug 1, 2009 · 4 comments
Closed

PyList_APPEND (append without incref) #50865

ideasman42 mannequin opened this issue Aug 1, 2009 · 4 comments
Assignees

Comments

@ideasman42
Copy link
Mannequin

ideasman42 mannequin commented Aug 1, 2009

BPO 6616
Nosy @rhettinger, @ideasman42
Files
  • py3_APPEND.diff: patch on r74276
  • 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/rhettinger'
    closed_at = <Date 2009-08-02.21:44:27.398>
    created_at = <Date 2009-08-01.20:43:45.779>
    labels = []
    title = 'PyList_APPEND (append without incref)'
    updated_at = <Date 2009-08-02.21:44:27.396>
    user = 'https://github.com/ideasman42'

    bugs.python.org fields:

    activity = <Date 2009-08-02.21:44:27.396>
    actor = 'rhettinger'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2009-08-02.21:44:27.398>
    closer = 'rhettinger'
    components = []
    creation = <Date 2009-08-01.20:43:45.779>
    creator = 'ideasman42'
    dependencies = []
    files = ['14619']
    hgrepos = []
    issue_num = 6616
    keywords = ['patch']
    message_count = 4.0
    messages = ['91167', '91169', '91179', '91207']
    nosy_count = 2.0
    nosy_names = ['rhettinger', 'ideasman42']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue6616'
    versions = ['Python 3.2']

    @ideasman42
    Copy link
    Mannequin Author

    ideasman42 mannequin commented Aug 1, 2009

    This patch was made on python r74276

    Often when writing in C/Python I want to append to a list within a C
    loop of an unknown length.
    When this is done for newly created PyObject (which is quite common) -
    you need to assign each item to a variable and then decref it.

    eg:
     PyObject *item= PyFloat_FromDouble(x);
     PyList_Append(list, item);
     Py_DECREF(item);

    I have seen people make mistakes with this (in pygame code and
    blender3d), and run PyList_Append(list, PyFloat_FromDouble(x)),
    ofcourse this is not the fault of python/c api that devs do not read
    docs properly but I think it would be very convenient to have an append
    that works in a similar way to PyList_SET_ITEM

    This simple patch allows...
    PyList_APPEND(list, PyFloat_FromDouble(x))

    doc included.

    @rhettinger
    Copy link
    Contributor

    This simple patch allows...
    PyList_APPEND(list, PyFloat_FromDouble(x))

    This isn't a good idea because the error checking
    for the inner function is lost. The current API
    encourages keeping one function per line so that
    the ref counting and error-checking are more obvious.

    Also, I'm not too keen on adding second-ways-to-do-it.
    The C API is already somewhat fat, making it harder
    to learn and remember. The current API has standard
    patterns of creating and consuming references.
    Introducing variants that don't follow those patterns
    makes it harder to review code and determine that it
    is correct.

    It is better to learn to use the API as designed than
    to commingle two different styles.

    @ideasman42
    Copy link
    Mannequin Author

    ideasman42 mannequin commented Aug 2, 2009

    Hi Raymond, in general I agree with your comments that adding 2 ways to
    do things is not great. The reason I still think this is an advantage is
    that I know the API well enough (with manipulating dicts/lists etc) and
    I still want this function for the sake of convenience.

    For the same reason that PyList_SET_ITEM is useful, this is useful too.

    Loosing the error checking is a disadvantage but in many cases API's
    dont do error checking for every new item allocated.

    Python's own code does this in a few cases with PyList_SET_ITEM...
    eg.
    ./Python/_warnings.c:857: PyList_SET_ITEM(filters, 1,
    create_filter(PyExc_ImportWarning, "ignore"))
    ./Python/Python-ast.c:2757: PyList_SET_ITEM(value, i,
    ast2obj_cmpop((cmpop_ty)asdl_seq_GET(o->v.Compare.ops, i)));

    @rhettinger
    Copy link
    Contributor

    I know the API well enough (with manipulating dicts/lists etc)
    and I still want this function for the sake of convenience.

    Given that your level of skill is higher than the average user,
    I recommend adding this to your own standard libraries or headers.
    No need to muck-up the environment for everyone else.

    @rhettinger rhettinger self-assigned this Aug 2, 2009
    @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
    Labels
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant