Enhance Argument Clinic's NoneType return converter to give void #91284

Activity

1. arhadthedev commented on Mar 26, 2022

   

   arhadthedev

   on Mar 26, 2022

   MemberAuthor

   More actions

   The attached PR makes the following possible (note that the impl has a void return type):

   /*[clinic input]
   _io._IOBase.writelines -> NoneType
       lines: object
       /
   [clinic start generated code]*/
   

       static void
       _io__IOBase_writelines_impl(PyObject *self, PyObject *lines)
       /*[clinic end generated code: output=f3feca36db72dbd1 input=286ba711cb7291ad]*/

   Previously, the return type would be Object * with generated replacement of non-Py_None values to NULL on the other side.

   So now there is no need to track whether NULL or Py_None should be returned. Or should it be Py_RETURN_NONE? Argument Clinic does it by itself returning NULL on errors and PyNone otherwise:

       static PyObject *
       _io__IOBase_writelines(PyObject *self, PyObject *lines)
       {
           PyObject *return_value = NULL;
   
           _io__IOBase_writelines_impl(self, lines);
           if (PyErr_Occurred()) {
               goto exit;
           }
           return_value = Py_None;
           Py_INCREF(Py_None);
   
       exit:
           return return_value;
       }

   👍React with 👍1Reacted by Noam Cohen
2. arhadthedev

   added

   3.11only security fixesonly security fixes

   topic-argument-clinic

   type-featureA feature request or enhancementA feature request or enhancement

   on Mar 26, 2022
3. serhiy-storchaka commented on Mar 26, 2022

   

   serhiy-storchaka

   on Mar 26, 2022

   Member

   More actions

   The function should return different values for success and error. Functions which do not do this have bad design.
4. arhadthedev commented on Mar 26, 2022

   

   arhadthedev

   on Mar 26, 2022

   MemberAuthor

   More actions

   The function should return different values for success and error

   It does, and a void return type enforces it. Here is the trick:

   An important convention throughout the Python interpreter is the following: when a function fails, it should set an exception condition and return an error value (usually -1 or a NULL pointer).

   • https://docs.python.org/3/extending/extending.html#intermezzo-errors-and-exceptions

   Previously, a function could return NULL but forget to call PyErr_*(). With void, however, the function has no other choise but to properly set the error indicator so the Clinic's part will see it and return NULL finishing the convention.
5. serhiy-storchaka commented on Mar 27, 2022

   

   serhiy-storchaka

   on Mar 27, 2022

   Member

   More actions

   It is simpler and faster to return NULL than call PyErr_Occurred(). There is a special macro PY_RETURN_NONE, so there is no problem with returning None either.

   I do not think it would make the code better.
6. serhiy-storchaka

   closed this as completedon Mar 27, 2022
7. serhiy-storchaka

   closed this as completedon Mar 27, 2022
8. arhadthedev commented on Mar 28, 2022

   

   arhadthedev

   on Mar 28, 2022

   MemberAuthor

   More actions

   Actually, you're right. For now, PyErr_Occurred is a GIL lock plus a memory access. While the access is cheap because of a L1 cache hit, the GIL takes its toll in a hot path.

   So I'm closing the PR until GIL removal is done so no performance penalty will be imposed.

   I could use _PyErr_Occurred because "Currently Argument Clinic is considered internal-only for CPython", but it requires extra modifications of the clinic that is undesirable.
9. ezio-melotti

   transferred this issue fromon Apr 10, 2022
10. arhadthedev commented on Jul 20, 2022

    

    arhadthedev

    on Jul 20, 2022

    Last edited by arhadthedev

    MemberAuthor

    More actions

    So I'm closing the PR until GIL removal is done so no performance penalty will be imposed.

    Not relevant anymore; see #32126 (comment).