Description

encukou

opened on Apr 13, 2023

Issue body actions

PEP 697 was accepted and needs to be implemented.

Linked PRs

• gh-103509: PEP 697 -- Limited C API for Extending Opaque Types #103511

Activity

1. encukou

   added

   type-featureA feature request or enhancementA feature request or enhancement

   on Apr 13, 2023
2. encukou

   self-assigned this

   on Apr 13, 2023
3. encukou

   added a commit that references this issue on Apr 13, 2023

   pythongh-103509: PEP 697 -- Limited C API for Extending Opaque Types

   a9679fa
4. encukou

   added a commit that references this issue on May 4, 2023

   gh-103509: PEP 697 -- Limited C API for Extending Opaque Types (GH-10…

   cd9a56c
5. encukou commented on May 4, 2023

   

   encukou

   on May 4, 2023

   MemberAuthor

   More actions

   Erlend mentioned that he'll have ideas for follow-up PRs, so keeping the issue open.

   Reacted by Erlend E. Aasland and Oleg Iarygin
6. erlend-aasland commented on May 4, 2023

   

   erlend-aasland

   on May 4, 2023

   Contributor

   More actions

   Erlend mentioned that he'll have ideas for follow-up PRs, so keeping the issue open.

   I'll just post my suggestion here instead of opening a PR. It kind of bothers me a bit that PyObject_GetTypeData and PyType_GetTypeDataSize currently always succeeds (and thus never raise an exception), but both are documented to raise an exception on error. I feel the urge to assert the current behaviour so we suddenly don't end up with them returning NULL or -1 without an exception set. What do you think, Petr, is it worth it?

   suggestion - diff

   diff --git a/Objects/typeobject.c b/Objects/typeobject.c
   index 171c76a59a..ad7b9c1ce4 100644
   --- a/Objects/typeobject.c
   +++ b/Objects/typeobject.c
   @@ -4532,7 +4532,12 @@ void *
    PyObject_GetTypeData(PyObject *obj, PyTypeObject *cls)
    {
        assert(PyObject_TypeCheck(obj, cls));
   -    return (char *)obj + _align_up(cls->tp_base->tp_basicsize);
   +    void *ret = (char *)obj + _align_up(cls->tp_base->tp_basicsize);
   +
   +    /* This API is documented to raise an exception if it returns NULL.
   +     * The current implementation, however, always succeeds. */
   +    assert(ret != NULL);
   +    return ret;
    }
    
    Py_ssize_t
   @@ -4542,6 +4547,10 @@ PyType_GetTypeDataSize(PyTypeObject *cls)
        if (result < 0) {
            return 0;
        }
   +
   +    /* This API is documented to raise an exception if it returns -1.
   +     * The current implementation, however, always succeeds. */
   +     assert(result >= 0);
        return result;
    }
7. sunmy2019 commented on May 4, 2023

   

   sunmy2019

   on May 4, 2023

   Member

   More actions

   I feel the urge to assert the current behaviour

   makes sense
8. encukou commented on May 4, 2023

   

   encukou

   on May 4, 2023

   MemberAuthor

   More actions

   It doesn't bother me.
   (char *)obj + _align_up(cls->tp_base->tp_basicsize) can obviously never be NULL, so the assert is superfluous. If a future dev sets it to something that can be NULL, they're responsible for also setting an exception -- as always when adding an error case. The fact that there's no existing error case looks irrelevant to me.

   We could add a comment for clarity, but I would word it as.

   /* The user must ensure that *cls* was created using
      negative PyType_Spec.basicsize. Currently we don't have a good way to check that.
      For errors that we can detect, this function should raise an exception and return NULL. */
   

   However, that's just repeating what's already in the docs...
9. erlend-aasland commented on May 4, 2023

   

   erlend-aasland

   on May 4, 2023

   Contributor

   More actions

   NP, it's not a deal-breaker for me. Let's just leave it as it is 🙂
10. erlend-aasland

    closed this as completedon May 4, 2023

Issue actions