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

Clarify which "identifiers" in the C API are macros #93733

Open
soegaard opened this issue Jun 11, 2022 · 19 comments
Open

Clarify which "identifiers" in the C API are macros #93733

soegaard opened this issue Jun 11, 2022 · 19 comments
Labels
docs Documentation in the Doc dir topic-C-API

Comments

@soegaard
Copy link

soegaard commented Jun 11, 2022

Documentation

The "Python/C API Reference Manual" is an excellent resource but could be even better
if C macros were marked as such.

For a programmer that writes directly in C it doesn't matter which identifiers are real C functions
and which are C macros. However, when Python is used through libpython only the
C functions are available. This is the case for languages embedding Python through ffilib.

As an example: Consider the C macro PyImport_ImportModuleEx and the C function PyImport_ImportModuleLevel. They are documented in a way that makes it impossible
to guess that one is a C macro.

PyObject *PyImport_ImportModuleEx(const char *name, PyObject *globals, PyObject *locals, PyObject *fromlist)

PyObject *PyImport_ImportModuleLevel(const char *name, PyObject *globals, PyObject *locals, PyObject *fromlist, int level)

A look in "Python.c" (or friends import.h here) will reveal that PyImport_ImportModuleEx is a C macro.
But it would be a quality of life-improvement, if a simple "C Macro" were added below the
signature of C macros.

This "C macro" annotation could have the same style as the "Return value is a new reference" annotation.
(maybe with a different color).

@soegaard soegaard added the docs Documentation in the Doc dir label Jun 11, 2022
@ghost
Copy link

ghost commented Jun 12, 2022

+1

In my opinion, even better would be the requirement that the C API functions, macros, and so on -- that are described in the documentation -- also have a corresponding link to the file in which they are defined. Of course, there are other ways to find where something is defined, but it could be very helpful if it were a part of the documentation.

The line number shouldn't be factored in since it would very quickly become invalidated through PRs that don't even touch these API items. The link to the file should only be changed whenever one wishes to add/remove an API item from a file (perhaps to relocate the definition to a different file).

I could be missing other considerations though.

@soegaard
Copy link
Author

The documentation is writting using "sphinx".
In the manual [1] I see that C functions are marked with "c:function".
However, the comment says:

This is also used to describe function-like preprocessor macros.

The annotation "c:macro" is described as:

Describes a “simple” C macro. Simple macros are macros which are used for code expansion, but which do not take arguments so cannot be described as functions.

Is step one to extend Sphinx with a new annotation "c:macrofunction" ?

[1] https://pvbookmarks.readthedocs.io/en/master/documentation/doc_generators/sphinx/rest_sphinx/domain/c_domain.html#c-function

@ericvsmith
Copy link
Member

Some of these (maybe all?) are deliberately left underspecified so we can switch if they're macros or functions.

@soegaard
Copy link
Author

In that case, I think, should state that fact explicitly.

Btw - I see it as a positive that there is an ongoing effort to convert macros into static, inline functions:
#89653

@ghost
Copy link

ghost commented Jun 13, 2022

If we were to specify whether it is a macro or a function, is there something documented or about the status quo that would dictate that it must stay that way for x amount of time? I think we mainly guarantee behavior (via the docs) and very rarely the implementation, no?

If not, I think the net benefit of distinguishing them on the documentation is positive, no?

@ericvsmith
Copy link
Member

There are no guarantees beyond the limited API and stable ABI. Maybe @vstinner can give his opinion.

@vstinner
Copy link
Member

Documentation patches are welcomed ;-)

@soegaard
Copy link
Author

@vstinner

Documentation patches are welcomed ;-)
Great to hear.

Before I make a patch, I need to know, what kind of change is wanted.

Take the C macro PyImport_ImportModuleEx as an example.
Currently the documentation source [1] looks like this:

.. c:function:: PyObject* PyImport_ImportModuleEx(const char *name, PyObject *globals, PyObject *locals, PyObject *fromlist)

   .. index:: builtin: __import__

   Import a module.  This is best described by referring to the built-in Python
   function :func:`__import__`.
   <omitted>

When I look at the documentation for Sphinx [2], I see:

    c:function:: function prototype
    Describes a C function. The signature should be given as in C, e.g.:
    <omitted>

This suggests that I should change c:function to something else.
I would like to change it to c:macro but @ericvsmith mentions that it
is unspecified whether PyImport_ImportModuleEx is a C function or a C macro.

The Right™ solution would be to add an c:function_or_macro annotation to Sphinx.

The faster way is to add an explanation in the text:

"Currently a C macro, but could change to a C function in the future."
.. c:function:: PyObject* PyImport_ImportModuleEx(const char *name, PyObject *globals, PyObject *locals, PyObject *fromlist)
    "Currently a C macro, but could change to a C function in the future."

   .. index:: builtin: __import__

   Import a module.  This is best described by referring to the built-in Python
   function :func:`__import__`.
   <omitted>

Is this phrasing acceptable - or is it better to avoid the word "currently".

[1] https://github.com/python/cpython/blob/main/Doc/c-api/import.rst

[2] https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html

@vstinner
Copy link
Member

I suggest adding "Function added as a macro." If it becomes a regular tomorrow, this sentence can be removed.

cc @encukou

@encukou
Copy link
Member

encukou commented Jun 16, 2022

well if you ask me...
IMO the correct solution is to change the code: add an actual exported function in addition to the macro.

@vstinner
Copy link
Member

IMO the correct solution is to change the code: add an actual exported function in addition to the macro.

Well, if you ask me, macros should be converted to functions: https://peps.python.org/pep-0670/ :-)

I converted a few macros to static inline functions, but not to regular functions yet. As said in PEP 670, there is a question about the performance cost of such change. Since there are very few users requesting to call functions currently implemented as macros or static inline functions, there is active work on converting these to regular functions. It's more done on a case by case basis. Well, in general, IMO the overhead is not significan or can be ignored.

@ghost
Copy link

ghost commented Jun 18, 2022

@soegaard Will you be making a PR? FWIW, if you wanted to just add to the text, I would suggest ``Function-like macro'' or going with Victor's suggestion.

Following Petr's suggestion, it may be worth creating a separate issue to see how other maintainers feel about creating actual functions in addition to the existing function-like macros in the C API to facilitate embedding Python. I wouldn't try to address this in your PR.

@soegaard
Copy link
Author

@oda-gitso

I'll make some PR, but first let me know if the example below looks okay.

I added the phrase "Function-like macro" to import.rst which
contains the documentation of PyImport_ImportModuleEx.
It is the only function-like macro in import.h.

I used your phrase "Function-like macro" since that's the term used in the C specification.

The rendered documentation looks like this:

image

Ideally I would have liked no line between "Return value: New reference." and "Function-like macro.",
but I think that requires modifying Sphinx to fix.

If this looks okay, I'll look at the other files and make a PR.

The diff is here:

main...soegaard:mark-function-like-macros

@soegaard
Copy link
Author

Here is a version with "Function-like macro" in italics.
I think it looks a little better.

image

@ghost
Copy link

ghost commented Jun 19, 2022

@soegaard Thanks for getting back to me and for posting screenshots of your suggestions. I agree with you that the ``Function-like macro'' info is best presented along with the signature and return value. If I had to choose between the two screenshots, I would go with the second.

@AA-Turner I was wondering if I could get your input on how to best present the ``Function-like macro'' info?

@encukou
Copy link
Member

encukou commented Jun 20, 2022

If this is documented, we should agree when this can be changed (function become a macro, and when a macro can become a function), and also document that in the devguide. (AFAICS, currently, such a change can't happen in a point release due to ABI stability needs; but it can happen in a minor release – 3.x.)
IMO, tests should also be added to ensure people don't let the docs go out of sync when they change what they see as a detail.

AFAICS, static inline functions have the same issue as macros for libpython users. Should they be documented differently? Should the note be more general, like Not exported?

Will users be able to tell the difference between an actual exported function, and a macro-function for which no one added the note yet? Or will there be some mechanism to ensure notes are always added where relevant?

(FWIW, I've been tackling similar issues in the Limited API: all of the functions there are actually exported, with tests to ensure they stay that way.)

@ghost
Copy link

ghost commented Jun 22, 2022

Thanks @encukou. All very good points.

...static inline functions have the same issue as macros for libpython users. Should they be documented differently? Should the note be more general, like Not exported?

I personally would prefer seeing Static inline function whenever it is a static inline function and Function-like macro whenever it is implemented as a macro. One concern is that exported is not a term used in the C Standard. Another is that the semantics differ and so there could be benefit to being a little bit more specific. However, I also like Not exported so I am not sure what is best.

Will users be able to tell the difference between an actual exported function, and a macro-function for which no one added the note yet? Or will there be some mechanism to ensure notes are always added where relevant?

My intuition says there should be some kind of mechanism to ensure that notes are always added where relevant though no obvious solution comes to mind. Of course, updating the devguide will help and over time maintainers will become accustomed to the new procedure but it is not foolproof.

Could you maybe point me to an example of how you are ''ensur[ing] they stay that way"? It might provide some ideas.

Thanks again!

@encukou
Copy link
Member

encukou commented Jun 22, 2022

One concern is that exported is not a term used in the C Standard.

AFAIK, the C standard doesn't cover dynamically linked libraries at all. The note would definitely need to be linked to an explanation, anyway.

Could you maybe point me to an example of how you are ''ensur[ing] they stay that way"? It might provide some ideas.

The current test does ctypes.pythonapi[symbol_name] to load each symbol from libpython: https://github.com/python/cpython/blob/fda4b2f06364ae5ef91ecd9c09e2af380c8b0b4c/Lib/test/test_stable_abi_ctypes.py
The limited API has a well-defined list of what's in it. That's unfortunately not the case with the whole C-API.

Of course, updating the devguide will help and over time maintainers will become accustomed to the new procedure but it is not foolproof.

My pessimistic view is that until it is made foolproof, it's better for you as the user to try loading the function from libpython rather than look in the docs.
Or stick to the limited API. I'm actually focusing on making that subset of the API more friendly to non-C languages. Feel free to add any Racket notes to the brainstorming doc.

@soegaard
Copy link
Author

@encukou wrote:

My pessimistic view is that until it is made foolproof, it's better for you as the user to try loading the function from libpython rather than look in the docs.

I have added a note to the document. When the bindings are done, I'll report back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir topic-C-API
Projects
None yet
Development

No branches or pull requests

5 participants