On Sun, Oct 28, 2018 at 10:20 AM Victor Stinner <vstin...@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 _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
