On Sun, Oct 28, 2018 at 10:20 AM Victor Stinner vstinner@redhat.com wrote:
Python C API has no strict separation between the 3 levels of API:
- core: Py_BUILD_CORE define
- stable: Py_LIMITED_API 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 Py_BUILD_CORE" and "#ifndef Py_LIMITED_API" 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/pycore_objimpl.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/pycore_objimpl.h if Py_BUILD_CORE is defined:
#ifdef Py_BUILD_CORE # include "pycore/pycore_objimpl.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/pycore_pygetopt.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/pycore_pygetopt.h" if Py_BUILD_CORE 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 Py_BUILD_CORE specific code into Include/pycore/pycore_*.h
+1
- Automatically include pycore files from Include/*.h files (#ifdef Py_BUILD_CORE)
-0 (maybe okay as an intermediate fix)
Second milestone:
- Find a solution for Py_LIMITED_API :-)
+1 :)
-eric