[Python-Dev] Rename Include/internals/ to Include/pycore/ (original) (raw)

Eric Snow ericsnowcurrently at gmail.com
Wed Oct 31 14:22:29 EDT 2018

On Sun, Oct 28, 2018 at 10:20 AM Victor Stinner <vstinner at redhat.com> wrote:

Python C API has no strict separation between the 3 levels of API:

* core: PyBUILDCORE define * stable: PyLIMITEDAPI define (it has a value) * regular: no define IMHO the current design of header files is done backward: by default, everything is public. To exclude an API from core or stable, "#ifdef PyBUILDCORE" and "#ifndef PyLIMITEDAPI" are used. This design caused issues in the past: functions, variables or something else exposed whereas they were supposed to be "private".

This is a great point.

I propose a practical solution for that: Include/*.h files would only be be public API.

As we've already discussed, I'm entirely in favor of this. :) In fact, I was thinking along those same lines when I originally created "Include/internal", when working on the runtime state consolidation. When you originally shared your idea with me (2 years ago?) it made perfect sense. :)

The "core" API would live in a new subdirectory: Include/pycore/*.h.

I'm mostly -0 on this. "pycore" is fine I suppose, but I think "internal" is more obvious to people looking through the repo. I can imagine folks getting confused by having "core" in the directory name.

Moreover, files of this subdirectory would have the prefix "pycore". For example, Include/objimpl.h would have a twin: Include/pycore/pycoreobjimpl.h which extend the public API with "core" APIs.

I'm not sure why this is necessary. When I created Include/internal I ran into some issues with relative includes. However, I solved them by doing the following (at Benjamin's recommendation, IIRC): always explicitly specify "internal/<...>.h" in includes, even in Include/internal/*.h.

For example:

https://github.com/python/cpython/blob/master/Include/internal/pystate.h#L11 #include "internal/ceval.h" Note that a few lines above in that file I include the public header file: #include "pystate.h"

Taking this approach there were no issues with "relative" includes. Incidentally, I originally tried to solve the problem of relative includes with a prefix ("_" IIRC), but that proved to introduce other issues. Switching to using the explicit "internal/" worked great and was more clear.

I also propose to automatically load the twin: Include/objimpl.h would load Include/pycore/pycoreobjimpl.h if PyBUILDCORE is defined:

#ifdef PyBUILDCORE # include "pycore/pycoreobjimpl.h" #endif

During the runtime state consolidation I took a similar approach initially, though at a less granular scale, which proved to be a headache. At first I added Include/internal/Python.h and then tried to conditionally include it in Include/Python.h. That ended up confusing, problematic, and unnecessary. At Benjamin's suggestion I switched to explicitly including "internal/<...>.h" in .c files (only where necessary). The result is simpler and more clear, identifying dependencies in source files more tightly. It's also a bit more idiomatic for well-written C code.

Here's an example of what I did:

https://github.com/python/cpython/blob/master/Python/ceval.c#L13 #include "internal/pystate.h"

Your proposal is similar, though more granular. And I suppose it is reasonable as a short-term step for moving internal API out of the public header files. However, I agree with Benjamin that the ideal would be to not have the public headers rely at all on the internal ones. I realize that I somewhat introduced the problem when I added the internal headers, so I certainly don't have a lot of room to ask anything here. :) Regardless, an intermediate step of conditionally including the private headers in the public ones might be okay if we have a long-term plan on how to not include the private headers at all. I'd be interested in discussing that.

FWIW, in the existing internal headers I include some of the public header files. I don't recall if I did anything the other direction (i.e. include internal headers conditionally in the public ones).

Only in some rare cases, you would have to explicitly use: #include "pycore/pycorepygetopt.h". This header is fully private, there is no public header in Include/pygetopt.h. Or maybe we should modify Include/Python.h to also include "pycore/pycorepygetopt.h" if PyBUILDCORE is defined? Well, that's just a detail.

As noted above, I tried that and it didn't work out well.

First milestone:

* Create Include/pycore/

-0 (less clear)

* Move PyBUILDCORE specific code into Include/pycore/pycore*.h

+1

* Automatically include pycore files from Include/*.h files (#ifdef PyBUILDCORE)

-0 (maybe okay as an intermediate fix)

Second milestone:

* Find a solution for PyLIMITEDAPI :-)

+1 :)

-eric

More information about the Python-Dev mailing list