Mailman 3 Rename Include/internals/ to Include/pycore/ - Python-Dev

Rename Include/internals/ to Include/pycore/

older
PEP 574 -- Pickle protocol 5 with...

Victor Stinner

28 Oct 2018 28 Oct '18

4:20 p.m.

Hi, 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". I propose a practical solution for that: Include/*.h files would only be be public API. The "core" API would live in a new subdirectory: Include/pycore/*.h. 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 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 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. First milestone: * Create Include/pycore/ * Move Py_BUILD_CORE specific code into Include/pycore/pycore_*.h * Automatically include pycore files from Include/*.h files (#ifdef Py_BUILD_CORE) Second milestone: * Find a solution for Py_LIMITED_API :-) Backward compatibility? The overall change is fully backward compatible. The default API doesn't change. C code (using header fles) doesn't have to be changed. Only a specific kinds of applications like debugger may have to be modified if they really have to access the "core" API, the "most private" API. Honestly, today it's unclear to me if this API can technically be used outside CPython. Victor

Show replies by date

Victor Stinner

28 Oct 28 Oct

4:23 p.m.

Oh, I forgot to add a reference to the bugs.python.org issue and my pull request! * https://bugs.python.org/issue35081 * https://github.com/python/cpython/pull/10143 My PR more or less implements the first milestone of my plan (Py_BUILD_CORE): it creates Include/pycore/. Victor Le dim. 28 oct. 2018 à 17:20, Victor Stinner a écrit :

...

Hi,

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".

I propose a practical solution for that: Include/*.h files would only be be public API. The "core" API would live in a new subdirectory: Include/pycore/*.h. 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 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

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.

First milestone:

* Create Include/pycore/ * Move Py_BUILD_CORE specific code into Include/pycore/pycore_*.h * Automatically include pycore files from Include/*.h files (#ifdef Py_BUILD_CORE)

Second milestone:

* Find a solution for Py_LIMITED_API :-)

Backward compatibility? The overall change is fully backward compatible. The default API doesn't change. C code (using header fles) doesn't have to be changed.

Only a specific kinds of applications like debugger may have to be modified if they really have to access the "core" API, the "most private" API. Honestly, today it's unclear to me if this API can technically be used outside CPython.

Victor

Benjamin Peterson

8:40 p.m.

On Sun, Oct 28, 2018, at 09:20, Victor Stinner wrote:

...

Hi,

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".

I propose a practical solution for that: Include/*.h files would only be be public API. The "core" API would live in a new subdirectory: Include/pycore/*.h. 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 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

I don't think more or less API should be magically included based on whether Py_BUILD_CORE is defined or not. If we want to have private headers, we should include them where needed and not install them. Really, Py_BUILD_CORE should go away. We should be moving away from monolithic includes like Python.h to having each C file include exactly what it uses, private or not.

Victor Stinner

9:30 p.m.

Le dim. 28 oct. 2018 à 21:50, Benjamin Peterson a écrit :

...

I don't think more or less API should be magically included based on whether Py_BUILD_CORE is defined or not. If we want to have private headers, we should include them where needed and not install them. Really, Py_BUILD_CORE should go away. We should be moving away from monolithic includes like Python.h to having each C file include exactly what it uses, private or not.

I would prefer to avoid annoying with the backward compatibility. Currently, #include more or less provides you "anything" and I'm fine with that. I prefer to no put too many changes at once :-) My overall approach is to make sure that we don't leak functions by mistakes into the public API or into the stable API anymore. For example, if a function is really for the core, put it in pycore/. It will be more explicit when reviewing a change for example. Py_BUILD_CORE is not only used to select which functions you get. Py_BUILD_CORE is also commonly used to get a macro instead of a function call, for best performances. Victor

Benjamin Peterson

29 Oct 29 Oct

5:32 a.m.

On Sun, Oct 28, 2018, at 14:30, Victor Stinner wrote:

...

Le dim. 28 oct. 2018 à 21:50, Benjamin Peterson a écrit :

...
I don't think more or less API should be magically included based on whether Py_BUILD_CORE is defined or not. If we want to have private headers, we should include them where needed and not install them. Really, Py_BUILD_CORE should go away. We should be moving away from monolithic includes like Python.h to having each C file include exactly what it uses, private or not.

I would prefer to avoid annoying with the backward compatibility. Currently, #include more or less provides you "anything" and I'm fine with that.

It doesn't break backward compatibility if you only make this required for new APIs.

...

I prefer to no put too many changes at once :-)

My overall approach is to make sure that we don't leak functions by mistakes into the public API or into the stable API anymore. For example, if a function is really for the core, put it in pycore/. It will be more explicit when reviewing a change for example.

How does the current Include/internal/ directory fail at accomplishing your goal?

...

Py_BUILD_CORE is not only used to select which functions you get. Py_BUILD_CORE is also commonly used to get a macro instead of a function call, for best performances.

I think the macro and function versions should just have different names then.

Victor Stinner

12:38 p.m.

Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson a écrit :

...

...
My overall approach is to make sure that we don't leak functions by mistakes into the public API or into the stable API anymore. For example, if a function is really for the core, put it in pycore/. It will be more explicit when reviewing a change for example.

How does the current Include/internal/ directory fail at accomplishing your goal?

Hum, let me understand how I came into this issue. I tried to convert _PyObject_GC_TRACK() macro to a static inline function. The macro uses "_PyGC_generation0" which is defined earlier as: "extern PyGC_Head *_PyGC_generation0;". Problem: this symbol has been removed when Eric Snow moved it into _PyRuntime which contains "#define _PyGC_generation0 _PyRuntime.gc.generation0". Hum, how is possible that _PyObject_GC_TRACK() of objimpl.h works as expected? It seems like all C file using this macro explicitly uses #include "internal/pystate.h" which uses #include "internal/mem.h". Oh. To me, it seems wrong that a function or macro defined in Include/objimpl.h requires an explicit #include "internal/pystate.h". objimpl.h should be self-sufficient. I started to hack Include/internals/*.h, but then I have been traped by #include which is relative: an include from Include/internals/ first looks for the included file in Include/internals/. It means that Include/internals/mem.h cannot include Include/objimpl.h if Include/internals/objimpl.h also exists. Well, I would like to reorganize all these headers to make them more consistent, and converting macros to static inline functions force me to fix dependencies between header files.

...

...
Py_BUILD_CORE is not only used to select which functions you get. Py_BUILD_CORE is also commonly used to get a macro instead of a function call, for best performances.

I think the macro and function versions should just have different names then.

I don't want to break the backward compatibility. I would like to make small steps towards a better API. Concrete example with pystate.h: /* Variable and macro for in-line access to current thread state */ /* Assuming the current thread holds the GIL, this is the PyThreadState for the current thread. */ #ifdef Py_BUILD_CORE # define _PyThreadState_Current _PyRuntime.gilstate.tstate_current # define PyThreadState_GET() \ ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current)) #else # define PyThreadState_GET() PyThreadState_Get() #endif We cannot remove PyThreadState_GET() from the Python C API. Removing any function is likely going to break multiple C extensions. I don't want to change too many things at once. My first intent is to convert _PyObject_GC_TRACK() into a static function, not to break the Python C API :-) Victor

Benjamin Peterson

30 Oct 30 Oct

1:59 a.m.

On Mon, Oct 29, 2018, at 05:38, Victor Stinner wrote:

...

Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson a écrit :

...
...
My overall approach is to make sure that we don't leak functions by mistakes into the public API or into the stable API anymore. For example, if a function is really for the core, put it in pycore/. It will be more explicit when reviewing a change for example.

How does the current Include/internal/ directory fail at accomplishing your goal?

Hum, let me understand how I came into this issue. I tried to convert _PyObject_GC_TRACK() macro to a static inline function. The macro uses "_PyGC_generation0" which is defined earlier as: "extern PyGC_Head *_PyGC_generation0;". Problem: this symbol has been removed when Eric Snow moved it into _PyRuntime which contains "#define _PyGC_generation0 _PyRuntime.gc.generation0".

Hum, how is possible that _PyObject_GC_TRACK() of objimpl.h works as expected?

It seems like all C file using this macro explicitly uses #include "internal/pystate.h" which uses #include "internal/mem.h". Oh.

To me, it seems wrong that a function or macro defined in Include/objimpl.h requires an explicit #include "internal/pystate.h". objimpl.h should be self-sufficient.

I agree. I would say nothing in Include/*.h should be including Include/internal/*.h. We should move things into the "public" headers as required to fix this.

...

I started to hack Include/internals/*.h, but then I have been traped by #include which is relative: an include from Include/internals/ first looks for the included file in Include/internals/. It means that Include/internals/mem.h cannot include Include/objimpl.h if Include/internals/objimpl.h also exists.

Yeah, that's unfortunate. I suppose you could use <> includes.

...

Well, I would like to reorganize all these headers to make them more consistent, and converting macros to static inline functions force me to fix dependencies between header files.

How does the your /pycore/ proposal fix this?

Victor Stinner

2:45 p.m.

Le mar. 30 oct. 2018 à 02:59, Benjamin Peterson a écrit :

...

...
To me, it seems wrong that a function or macro defined in Include/objimpl.h requires an explicit #include "internal/pystate.h". objimpl.h should be self-sufficient.

I agree. I would say nothing in Include/*.h should be including Include/internal/*.h. We should move things into the "public" headers as required to fix this.

Hum. It seems like I'm trying to fix too many issues at the same time, and I'm confused by different constraints which may be incompatible. I started to cleanup and fix header files with the most "straightforward" changes: https://bugs.python.org/issue35081 I will see later if Include/internal/ still has to be rename or moved (Serhiy Storchaka proposed to move internal/*.h header files out of the Include/ directory, I'm not sure that it's really needed). Victor

Eric Snow

31 Oct 31 Oct

7:39 p.m.

On Mon, Oct 29, 2018 at 6:39 AM Victor Stinner wrote:

...

Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson a écrit :

...
How does the current Include/internal/ directory fail at accomplishing your goal?

Hum, let me understand how I came into this issue. I tried to convert _PyObject_GC_TRACK() macro to a static inline function. The macro uses "_PyGC_generation0" which is defined earlier as: "extern PyGC_Head *_PyGC_generation0;". Problem: this symbol has been removed when Eric Snow moved it into _PyRuntime which contains "#define _PyGC_generation0 _PyRuntime.gc.generation0".

(FTR, _PyGC_generation0 is in Include/internal/mem.h.)

...

Hum, how is possible that _PyObject_GC_TRACK() of objimpl.h works as expected?

It seems like all C file using this macro explicitly uses #include "internal/pystate.h" which uses #include "internal/mem.h". Oh.

To me, it seems wrong that a function or macro defined in Include/objimpl.h requires an explicit #include "internal/pystate.h". objimpl.h should be self-sufficient.

I agree. :) FWIW, when I consolidated the runtime state I took the approach of re-using existing symbols (where possible) to avoid churn. That's why the code does not reference _PyRuntime directly. So, the issue is that public header implicitly requires that the source file include the internal headers in order to get the symbol. That is definitely too convoluted. However, it seems like a consequence of the public header file relying on the internal runtime state, which is what you're trying to fix. Shouldn't _PyObject_GC_TRACK() be moved to an internal include file? IIRC, there were several similar cases where API (under #ifndef Py_LIMITED_API) relies on internal runtime state and should probably be moved out of the public headers. Any such cases should be fixed. If any public API must rely on the internal runtime state then it shouldn't rely on it directly like it currently does. Instead that state should be hidden behind public API that the public headers can access. Right?

...

[snip]

...
...
Py_BUILD_CORE is not only used to select which functions you get. Py_BUILD_CORE is also commonly used to get a macro instead of a function call, for best performances.

I think the macro and function versions should just have different names then.

I don't want to break the backward compatibility. I would like to make small steps towards a better API. Concrete example with pystate.h:

/* Variable and macro for in-line access to current thread state */

/* Assuming the current thread holds the GIL, this is the PyThreadState for the current thread. */ #ifdef Py_BUILD_CORE # define _PyThreadState_Current _PyRuntime.gilstate.tstate_current # define PyThreadState_GET() \ ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current)) #else # define PyThreadState_GET() PyThreadState_Get() #endif

This has a similar problem to _PyObject_GC_TRACK(): it is "public" API that relies on internal runtime state and on the source code including the internal headers.

...

We cannot remove PyThreadState_GET() from the Python C API. Removing any function is likely going to break multiple C extensions.

Right. There should be very few cases of public API that relies on internal details. In this case, could we split things up? For example: "Include/pystate.h" // Under Py_BUILD_CORE this is replaced. #define PyThreadState_GET() PyThreadState_Get() "Include/internal/pystate.h" #undef PyThreadState_GET #define _PyThreadState_Current _PyRuntime.gilstate.tstate_current #define PyThreadState_GET() \ ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current)) -eric

Victor Stinner

8:05 p.m.

Le mer. 31 oct. 2018 à 20:40, Eric Snow a écrit :

...

On Mon, Oct 29, 2018 at 6:39 AM Victor Stinner wrote:

...
Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson a écrit :

...
How does the current Include/internal/ directory fail at accomplishing your goal?

Hum, let me understand how I came into this issue. I tried to convert _PyObject_GC_TRACK() macro to a static inline function. The macro uses "_PyGC_generation0" which is defined earlier as: "extern PyGC_Head *_PyGC_generation0;". Problem: this symbol has been removed when Eric Snow moved it into _PyRuntime which contains "#define _PyGC_generation0 _PyRuntime.gc.generation0".

(FTR, _PyGC_generation0 is in Include/internal/mem.h.)

I pushed a first commit to "cleanup and fix" pystate.h: https://github.com/python/cpython/commit/9204fb8623896cc5f68ae350784ee25e8a7... * Remove _PyThreadState_Current * Replace _PyThreadState_Current with _PyRuntime.gilstate.tstate_current I prefer to be explicit about the reference to _PyRuntime. For example, in my text editor, I'm able to follow the reference to _PyRuntime. In a debugger, I'm also able to display "_PyRuntime.gilstate.tstate_current", whereas gdb fails to display "_PyThreadState_Current" value since it's a preprocessor only thing.

...

...
Hum, how is possible that _PyObject_GC_TRACK() of objimpl.h works as expected?

It seems like all C file using this macro explicitly uses #include "internal/pystate.h" which uses #include "internal/mem.h". Oh.

To me, it seems wrong that a function or macro defined in Include/objimpl.h requires an explicit #include "internal/pystate.h". objimpl.h should be self-sufficient.

I agree. :) FWIW, when I consolidated the runtime state I took the approach of re-using existing symbols (where possible) to avoid churn. That's why the code does not reference _PyRuntime directly.

Well, we can now fix these issues.

...

Shouldn't _PyObject_GC_TRACK() be moved to an internal include file?

I'm trying to do that, but I have the #include bug: if I have Include/objimpl.h and Include/internal/objimpl.h, the compiler is confused and picked the wrong file... And I cannot use #include "objimpl.h" to include Include/objdump.h from Include/internal/objdump.h. ... Again, that's why I would like to rename header files. I will be even worse when I will add a second directory for the stable ABI (second part of my plan).

...

IIRC, there were several similar cases where API (under #ifndef Py_LIMITED_API) relies on internal runtime state and should probably be moved out of the public headers. Any such cases should be fixed.

Yep. I'm trying to fix issues one by one. I started with the simplest changes.

...

If any public API must rely on the internal runtime state then it shouldn't rely on it directly like it currently does. Instead that state should be hidden behind public API that the public headers can access. Right?

Right. Internal things must be moved to Include/internal/.

...

This has a similar problem to _PyObject_GC_TRACK(): it is "public" API that relies on internal runtime state and on the source code including the internal headers.

_PyObject_GC_TRACK() is fine: since it's a private API, we can move it to Include/internal/ and require an explicit include of the internal header file.

...

...
We cannot remove PyThreadState_GET() from the Python C API. Removing any function is likely going to break multiple C extensions.

Right. There should be very few cases of public API that relies on internal details. In this case, could we split things up? For example:

"Include/pystate.h"

// Under Py_BUILD_CORE this is replaced. #define PyThreadState_GET() PyThreadState_Get()

"Include/internal/pystate.h"

#undef PyThreadState_GET #define _PyThreadState_Current _PyRuntime.gilstate.tstate_current #define PyThreadState_GET() \ ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))

This API is really the worse. If we do that, it's unclear if we get the optimized flavor or the PyThreadState_Get() function call... It depends if "Include/internal/pystate.h" is included or not!? My idea here is to add a new _PyThreadState_GET() macro which would only be available in the private API. It would make sure that we pick the right implementation. Maybe PyThreadState_GET() can always be an alias to PyThreadState_Get(). Just make sure that internals use _PyThreadState_GET(). Victor

Eric Snow

9:39 p.m.

On Wed, Oct 31, 2018 at 2:05 PM Victor Stinner wrote:

...

Le mer. 31 oct. 2018 à 20:40, Eric Snow a écrit :

...
On Mon, Oct 29, 2018 at 6:39 AM Victor Stinner wrote:

...
Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson a écrit :

...
How does the current Include/internal/ directory fail at accomplishing your goal?

Hum, let me understand how I came into this issue. I tried to convert _PyObject_GC_TRACK() macro to a static inline function. The macro uses "_PyGC_generation0" which is defined earlier as: "extern PyGC_Head *_PyGC_generation0;". Problem: this symbol has been removed when Eric Snow moved it into _PyRuntime which contains "#define _PyGC_generation0 _PyRuntime.gc.generation0".

(FTR, _PyGC_generation0 is in Include/internal/mem.h.)

I pushed a first commit to "cleanup and fix" pystate.h: https://github.com/python/cpython/commit/9204fb8623896cc5f68ae350784ee25e8a7...

Thanks for that. I don't have much time to review (typing w/left hand, sleeping babe in right), so it may make sense to get feedback from others (Nick?) before going too far down this road, just for a sanity check.

...

* Remove _PyThreadState_Current * Replace _PyThreadState_Current with _PyRuntime.gilstate.tstate_current

I prefer to be explicit about the reference to _PyRuntime. For example, in my text editor, I'm able to follow the reference to _PyRuntime. In a debugger, I'm also able to display "_PyRuntime.gilstate.tstate_current", whereas gdb fails to display "_PyThreadState_Current" value since it's a preprocessor only thing.

As long as we avoid pulling internal API into public header files, which is exactly the point of your efforts. :)

...

...
...
Hum, how is possible that _PyObject_GC_TRACK() of objimpl.h works as expected?

It seems like all C file using this macro explicitly uses #include "internal/pystate.h" which uses #include "internal/mem.h". Oh.

To me, it seems wrong that a function or macro defined in Include/objimpl.h requires an explicit #include "internal/pystate.h". objimpl.h should be self-sufficient.

I agree. :) FWIW, when I consolidated the runtime state I took the approach of re-using existing symbols (where possible) to avoid churn. That's why the code does not reference _PyRuntime directly.

Well, we can now fix these issues.

...

...
Shouldn't _PyObject_GC_TRACK() be moved to an internal include file?

I'm trying to do that, but I have the #include bug: if I have Include/objimpl.h and Include/internal/objimpl.h, the compiler is confused and picked the wrong file... And I cannot use #include "objimpl.h" to include Include/objdump.h from Include/internal/objdump.h.

... Again, that's why I would like to rename header files.

I will be even worse when I will add a second directory for the stable ABI (second part of my plan).

...

...
IIRC, there were several similar cases where API (under #ifndef Py_LIMITED_API) relies on internal runtime state and should probably be moved out of the public headers. Any such cases should be fixed.

Yep. I'm trying to fix issues one by one. I started with the simplest changes.

...

...
If any public API must rely on the internal runtime state then it shouldn't rely on it directly like it currently does. Instead that state should be hidden behind public API that the public headers can access. Right?

Right. Internal things must be moved to Include/internal/.

OK, good.

...

...
This has a similar problem to _PyObject_GC_TRACK(): it is "public" API that relies on internal runtime state and on the source code including the internal headers.

_PyObject_GC_TRACK() is fine: since it's a private API, we can move it to Include/internal/ and require an explicit include of the internal header file.

...
...
We cannot remove PyThreadState_GET() from the Python C API. Removing any function is likely going to break multiple C extensions.

Right. There should be very few cases of public API that relies on internal details. In this case, could we split things up? For example:

"Include/pystate.h"

// Under Py_BUILD_CORE this is replaced. #define PyThreadState_GET() PyThreadState_Get()

"Include/internal/pystate.h"

#undef PyThreadState_GET #define _PyThreadState_Current _PyRuntime.gilstate.tstate_current #define PyThreadState_GET() \ ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))

This API is really the worse. If we do that, it's unclear if we get the optimized flavor or the PyThreadState_Get() function call... It depends if "Include/internal/pystate.h" is included or not!?

Fair enough. we just need a way to provide the optimized impl without depending on internal API (e.g. _PyRuntime) in public header files, right? There should only be two or three cases where it's an issue (i.e. involving actual public API).

...

My idea here is to add a new _PyThreadState_GET() macro which would only be available in the private API. It would make sure that we pick the right implementation.

Maybe PyThreadState_GET() can always be an alias to PyThreadState_Get(). Just make sure that internals use _PyThreadState_GET().

+1 FWIW, I think you're on the right track with all this. :) -eric

...

Victor

Neil Schemenauer

2 Nov 2 Nov

5:22 p.m.

On 2018-10-28, Benjamin Peterson wrote:

...

I don't think more or less API should be magically included based on whether Py_BUILD_CORE is defined or not.

I agree.

...

If we want to have private headers, we should include them where needed and not install them. Really, Py_BUILD_CORE should go away. We should be moving away from monolithic includes like Python.h to having each C file include exactly what it uses, private or not.

It seems that is best practice (e.g. look at Linux kernel include file style). I wonder however what are the real benefits to having modular include files and directly using them as needed? Pros for modular includes: - speeds up build process if you have good dependency info in the build system. Right now, change Python.h and everything gets rebuilt. I'm not sure this is a huge advantage anymore. - makes it clearer where an API is implemented? Cons: - more work to include the correct headers - build system dependency definitions are more complicated. Other systems generally have automatic dependancy generates (i.e. parse C files and find used includes). A simple approach would be to introduce something like Python-internal.h. If you are a Python internal unit, you can include both Python.h and Python-internal.h. We could, over time, split Python-iternal.h into smaller modular includes.

Victor Stinner

7:01 p.m.

Le ven. 2 nov. 2018 à 18:32, Neil Schemenauer a écrit :

...

A simple approach would be to introduce something like Python-internal.h. If you are a Python internal unit, you can include both Python.h and Python-internal.h. We could, over time, split Python-iternal.h into smaller modular includes.

Since this discussion, I already moved most Py_BUILD_CORE in Include/internal/. I added a lot of #include "pycore_xxx.h" in C files. I started to reach the limit with this PR which adds the pycore_object.h include to not less than 33 C files: https://github.com/python/cpython/pull/10272 I rewrote this PR to avoid the need to modify 33 C files: https://github.com/python/cpython/pull/10276/files I added the following code to Python.h: #ifdef Py_BUILD_CORE /* bpo-35081: Automatically include pycore_object.h in Python.h, to avoid to have to add an explicit #include "pycore_object.h" to each C file. */ # include "pycore_object.h" #endif I'm not sure of it's a temporary workaround or not :-) Maybe a Python-internal.h would be a better solution? We can identify the most common header files needed to access "Python internals" and put them in this "common" header file. For example, most C files using Python internals have to access _PyRuntime global variable (55 C files), so "pycore_state.h" is a good candidate. By the way, I don't understand the rationale to have _PyRuntime in pycore_state.h. IMHO pycore_state.h should only contain functions to get the Python thread and Python interpreter states. IMHO _PyRuntime is unrelated and should belong to a different header file, maybe pycore_runtime.h? I didn't do this change yet *because* I would have to modify the 55 files currently including it. Victor

Eric Snow

6 Nov 6 Nov

6:13 p.m.

On Fri, Nov 2, 2018 at 1:02 PM Victor Stinner wrote:

...

Le ven. 2 nov. 2018 à 18:32, Neil Schemenauer a écrit :

...
A simple approach would be to introduce something like Python-internal.h. If you are a Python internal unit, you can include both Python.h and Python-internal.h. We could, over time, split Python-iternal.h into smaller modular includes.

Since this discussion, I already moved most Py_BUILD_CORE in Include/internal/. I added a lot of #include "pycore_xxx.h" in C files.

I started to reach the limit with this PR which adds the pycore_object.h include to not less than 33 C files: https://github.com/python/cpython/pull/10272

I rewrote this PR to avoid the need to modify 33 C files: https://github.com/python/cpython/pull/10276/files

I added the following code to Python.h:

#ifdef Py_BUILD_CORE /* bpo-35081: Automatically include pycore_object.h in Python.h, to avoid to have to add an explicit #include "pycore_object.h" to each C file. */ # include "pycore_object.h" #endif

I'm not sure of it's a temporary workaround or not :-) Maybe a Python-internal.h would be a better solution? We can identify the most common header files needed to access "Python internals" and put them in this "common" header file.

For example, most C files using Python internals have to access _PyRuntime global variable (55 C files), so "pycore_state.h" is a good candidate.

FWIW, I'm still in favor of keeping the includes specific. For me personally it helps to narrow down dependencies, making the source easier to follow.

...

By the way, I don't understand the rationale to have _PyRuntime in pycore_state.h. IMHO pycore_state.h should only contain functions to get the Python thread and Python interpreter states. IMHO _PyRuntime is unrelated and should belong to a different header file, maybe pycore_runtime.h? I didn't do this change yet *because* I would have to modify the 55 files currently including it.

The runtime state isn't all that different from the thread state or the interpreter state. It's simply broader in scope, much like the interpreter state is broader in scope than the thread state. That's why I put it where I did. -eric

Michael Felt

29 Oct 29 Oct

8:40 p.m.

My short comment: +1 My longer comment: for someone who is not trying to be caught up in "internals" I find it extremely difficult to work with the "default" approach described below - trying to mentally understand, and remember what those macros mean/do as I got "bug-hunting". Ultimately, have a clear-cut division between "public" and "internal" make it much much easier for "the rest of us" to know our boundaries and thereby develop code that is based on the "published" aka public API and not a (guess what I found) internal trick. Isn´t that why there is a "public" API to begin with. Where, or how the separation is provided is not the key issue. But being committed to it is, and this sounds like a step towards commitment and clarity.

...

On 10/28/2018 6:20 PM, Victor Stinner wrote: 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".

I propose a practical solution for that: Include/*.h files would only be be public API. The "core" API would live in a new subdirectory: Include/pycore/*.h. 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.

Eric Snow

31 Oct 31 Oct

6:22 p.m.

On Sun, Oct 28, 2018 at 10:20 AM Victor Stinner 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

...

* 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

Victor Stinner

7:18 p.m.

Le mer. 31 oct. 2018 à 19:22, Eric Snow a écrit :

...

...
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. :)

I think we discussed that during the CPython sprint in September 2017 :-) But I only formalized a full plan at the sprint in September 2018: http://pythoncapi.readthedocs.io/

...

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

I'm mostly -0 on this. "pycore" is fine I suppose (...)

Ok.

...

...
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. (...) 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"

The last include doesn't work: https://bugs.python.org/issue35081#msg328942 """ Include/internal/pystate.h uses #include "pystate.h" to include Include/pystate.h, but it tries to include itself (Include/internal/pystate.h) which does nothing because of "#ifndef Py_INTERNAL_PYSTATE_H #define Py_INTERNAL_PYSTATE_H ... #endif". Remove the #ifndef #define to see the bug: (...) Compilation fails with: In file included from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, ... ./Include/internal/pystate.h:5:21: error: #include nested too deeply #include "pystate.h" """ Using has no effect, it stills looks for Include/internal/pystate.h. IMHO the only solution is to use different names in Include/ and Include/internal/, at least for the header files of Include/internal/ which also exist in Include/. Rename Include/internal/pystate.h to Include/internal/pyruntime.h, Include/internal/internal_pystate.h or Include/internal/pycore_pystate.h. If we add Include/internal/ to the header search path (gcc -I Include/internal/), we can avoid redundant "internal/internal_<name>.h" and just write "internal_<name>.h". I proposed to rename "internal" to "pycore" to get: #include "pycore_pystate.h" instead of #include "internal_pystate.h". But I have no strong preference for "pycore" or "internal", both work for me.

...

...
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.

Ok, let's try this approach. I have to add many #include, but I'm fine to be very explicit about includes. One example (internal/mem.h): https://github.com/python/cpython/pull/10249 I'm writing "try" rather than "do", because something Py_BUILD_CORE core is mixed with public headers. Like #ifdef Py_BUILD_CORE ... #else ... #endif. See datetime.h for an example. It's not easy to separate both implementations. https://github.com/python/cpython/pull/10238

...

...
Second milestone:

* Find a solution for Py_LIMITED_API :-)

My current idea is to: * Remove all "#ifndef Py_LIMITED_API" and "#ifdef Py_BUILD_CORE" from Include/*.h * Move "#ifndef Py_LIMITED_API" code to Include/public/ * Include "Include/public/<name>.h" from "Include/<name>.h" IMHO here we really want to automatically include "Include/public/<name>.h" from "Include/<name>.h" to not impact the public API in any way. Py_BUILD_CORE is very different: it's only consumed inside Python, so it's fine to "break the API" and force to add new includes. Victor

Victor Stinner

7:57 p.m.

Ok, I proposed to rename internal header files, add an "internal_" prefix, to avoid this issue: https://github.com/python/cpython/pull/10263 My PR also adds Include/internal/ to search paths of header files. Extract of the change: diff --git a/Include/internal/pystate.h b/Include/internal/internal_pystate.h --- a/Include/internal/pystate.h +++ b/Include/internal/internal_pystate.h @@ -7,9 +7,9 @@ extern "C" { #include "pystate.h" #include "pythread.h" -#include "internal/mem.h" -#include "internal/ceval.h" -#include "internal/warnings.h" +#include "internal_pymem.h" +#include "internal_ceval.h" +#include "internal_warnings.h" With this PR, internal_pystate.h of Include/internal/ is able to properly include pystate.h from Include/ (include the correct header file). For practical reasons, I added Include/internal/ to all projects of the Visual Studio solution (I modified the properties common to all projects). Again, this is where I would like to replace "internal_" with "pycore_": "internal" is too generic, there is a risk that a third party project also the same header filename. "internal_hash.h" name is quite general. There is a non-zero risk of conflict with a library. In the past, we also had issues when compiling OpenSSL for example. Maybe we can keep "Include/internal/" directory name, but add "pycore_" rather than "internal_" to header filenames? Victor

...

...
...
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. (...) 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"

The last include doesn't work: https://bugs.python.org/issue35081#msg328942

""" Include/internal/pystate.h uses #include "pystate.h" to include Include/pystate.h, but it tries to include itself (Include/internal/pystate.h) which does nothing because of "#ifndef Py_INTERNAL_PYSTATE_H #define Py_INTERNAL_PYSTATE_H ... #endif".

Remove the #ifndef #define to see the bug: (...)

Compilation fails with:

In file included from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, from ./Include/internal/pystate.h:5, ... ./Include/internal/pystate.h:5:21: error: #include nested too deeply #include "pystate.h" """

Using has no effect, it stills looks for Include/internal/pystate.h.

IMHO the only solution is to use different names in Include/ and Include/internal/, at least for the header files of Include/internal/ which also exist in Include/.

Rename Include/internal/pystate.h to Include/internal/pyruntime.h, Include/internal/internal_pystate.h or Include/internal/pycore_pystate.h.

If we add Include/internal/ to the header search path (gcc -I Include/internal/), we can avoid redundant "internal/internal_<name>.h" and just write "internal_<name>.h". I proposed to rename "internal" to "pycore" to get: #include "pycore_pystate.h" instead of #include "internal_pystate.h". But I have no strong preference for "pycore" or "internal", both work for me.

Eric Snow

9:18 p.m.

On Wed, Oct 31, 2018 at 1:57 PM Victor Stinner wrote:

...

Ok, I proposed to rename internal header files, add an "internal_" prefix, to avoid this issue: https://github.com/python/cpython/pull/10263

My PR also adds Include/internal/ to search paths of header files.

Extract of the change:

diff --git a/Include/internal/pystate.h b/Include/internal/internal_pystate.h --- a/Include/internal/pystate.h +++ b/Include/internal/internal_pystate.h @@ -7,9 +7,9 @@ extern "C" {

#include "pystate.h" #include "pythread.h"

-#include "internal/mem.h" -#include "internal/ceval.h" -#include "internal/warnings.h" +#include "internal_pymem.h" +#include "internal_ceval.h" +#include "internal_warnings.h"

With this PR, internal_pystate.h of Include/internal/ is able to properly include pystate.h from Include/ (include the correct header file).

For practical reasons, I added Include/internal/ to all projects of the Visual Studio solution (I modified the properties common to all projects).

Again, this is where I would like to replace "internal_" with "pycore_": "internal" is too generic, there is a risk that a third party project also the same header filename. "internal_hash.h" name is quite general. There is a non-zero risk of conflict with a library. In the past, we also had issues when compiling OpenSSL for example.

I'm not sure of this being a real problem, but I don't see a problem with mitigating the risk. :)

...

Maybe we can keep "Include/internal/" directory name, but add "pycore_" rather than "internal_" to header filenames?

this sounds good to me. thanks for chasing this down. -eric

Victor Stinner

10:27 p.m.

Le mer. 31 oct. 2018 à 22:19, Eric Snow a écrit :

...

...
Maybe we can keep "Include/internal/" directory name, but add "pycore_" rather than "internal_" to header filenames?

this sounds good to me. thanks for chasing this down.

What do you prefer? (A Include/internal/internal_pystate.h (B) Include/internal/pycore_pystate.h (C) Include/pycore/pycore_pystate.h Victor

Eric Snow

10:37 p.m.

B On Wed, Oct 31, 2018 at 4:28 PM Victor Stinner wrote:

...

Le mer. 31 oct. 2018 à 22:19, Eric Snow a écrit :

...
...
Maybe we can keep "Include/internal/" directory name, but add "pycore_" rather than "internal_" to header filenames?

this sounds good to me. thanks for chasing this down.

What do you prefer?

(A Include/internal/internal_pystate.h (B) Include/internal/pycore_pystate.h (C) Include/pycore/pycore_pystate.h

Victor

Victor Stinner

11:36 p.m.

Ok, thanks. I decided to remove the redundant "py", so I renamed "pystate.h" to "pycore_state.h" (single "py", instead of "pycore_pystate.h", double "py py"). I updated my PR: https://github.com/python/cpython/pull/10263 * Rename Include/internal/ header files: * pyatomic.h -> pycore_atomic.h * ceval.h -> pycore_ceval.h * condvar.h -> pycore_condvar.h * context.h -> pycore_context.h * pygetopt.h -> pycore_getopt.h * gil.h -> pycore_gil.h * hamt.h -> pycore_hamt.h * hash.h -> pycore_hash.h * mem.h -> pycore_mem.h * pystate.h -> pycore_state.h * warnings.h -> pycore_warnings.h * PCbuild project, Makefile.pre.in, Modules/Setup: add the Include/internal/ directory to the search paths of header files. * Update include. For example, replace #include "internal/mem.h" with #include "pycore_mem.h". Victor Le mer. 31 oct. 2018 à 23:38, Eric Snow a écrit :

...

B On Wed, Oct 31, 2018 at 4:28 PM Victor Stinner wrote:

...
Le mer. 31 oct. 2018 à 22:19, Eric Snow a écrit :

...
...
Maybe we can keep "Include/internal/" directory name, but add "pycore_" rather than "internal_" to header filenames?

this sounds good to me. thanks for chasing this down.

What do you prefer?

(A Include/internal/internal_pystate.h (B) Include/internal/pycore_pystate.h (C) Include/pycore/pycore_pystate.h

Victor

Eric Snow

1 Nov 1 Nov

1:35 a.m.

On the one hand dropping redundancy in the filename is fine. On the other having the names mirror the public header files is valuable. How about leaving the base names alone and change the directory to "pyinternal"? -eric On Wed, Oct 31, 2018, 17:36 Victor Stinner

...

Ok, thanks. I decided to remove the redundant "py", so I renamed "pystate.h" to "pycore_state.h" (single "py", instead of "pycore_pystate.h", double "py py").

I updated my PR: https://github.com/python/cpython/pull/10263

* Rename Include/internal/ header files:

* pyatomic.h -> pycore_atomic.h * ceval.h -> pycore_ceval.h * condvar.h -> pycore_condvar.h * context.h -> pycore_context.h * pygetopt.h -> pycore_getopt.h * gil.h -> pycore_gil.h * hamt.h -> pycore_hamt.h * hash.h -> pycore_hash.h * mem.h -> pycore_mem.h * pystate.h -> pycore_state.h * warnings.h -> pycore_warnings.h

* PCbuild project, Makefile.pre.in, Modules/Setup: add the Include/internal/ directory to the search paths of header files. * Update include. For example, replace #include "internal/mem.h" with #include "pycore_mem.h".

Victor Le mer. 31 oct. 2018 à 23:38, Eric Snow a écrit :

...
B On Wed, Oct 31, 2018 at 4:28 PM Victor Stinner

wrote:

...
...
Le mer. 31 oct. 2018 à 22:19, Eric Snow

a écrit :

...
...
...
Maybe we can keep "Include/internal/" directory name, but add "pycore_" rather than "internal_" to header filenames?

this sounds good to me. thanks for chasing this down.

What do you prefer?

(A Include/internal/internal_pystate.h (B) Include/internal/pycore_pystate.h (C) Include/pycore/pycore_pystate.h

Victor

Victor Stinner

2:40 a.m.

I pushed many changes. I moved most of the Py_BUILD_CORE code out of Include/ header files. Le jeu. 1 nov. 2018 à 02:35, Eric Snow a écrit :

...

On the one hand dropping redundancy in the filename is fine. On the other having the names mirror the public header files is valuable.

Current content of Include/internal/: pycore_accu.h pycore_atomic.h pycore_ceval.h pycore_condvar.h pycore_context.h pycore_getopt.h pycore_gil.h pycore_hamt.h pycore_hash.h pycore_lifecycle.h pycore_mem.h pycore_pathconfig.h pycore_state.h pycore_warnings.h I tried to find a kind of consistency. For example, there was Include/internal/mem.h vs Internal/pymem.h. I renamed Include/pyatomic.h to Include/internal/pycore_atomic.h. Previously, the file had a "py" prefix to avoid the risk of having an header file coming from somewhere else, but now with the "pycore_" prefix, it's very unlikely. I renamed Include/accu.h to Include/internal/pycore_accu.h. There are 4 header files which are in Include/ and Include/internal/ and the one in internal has no "py": * pyhash.h and pycore_hash.h * pylifecycle.h and pycore_lifecycle.h * pymem.h and pycore_mem.h * pystate.h and pycore_state.h I wrote a PR to rename these 4 header files: https://github.com/python/cpython/pull/10275

...

How about leaving the base names alone and change the directory to "pyinternal"?

The name of the directory doesn't matter to fix the #include bug when two header files have the same filename in two directories (especially when Include/ and Include/internal/ have the same filename). Note: Since I added Include/internal/ to the search paths for header files, the name of the directory doesn't matter anymore. C code only uses the filename without "internal/" to include an internal header: #include "pycore_mem.h". Benjamin and you were opposed to change the name, so I kept "Include/internal/". Victor

Steve Dower

3:38 p.m.

I assume the redundancy was there to handle the naming collision problem, but a better way to do that is to have only header files that users should ever use in "include/", and put the rest inside "include/python/" and "include/internal/" (and if possible, we don't distribute the internal directory as part of -develop packages, which only really impacts the Windows installer as far as we're concerned, but I assume some distros will care). That would mean a layout like: include/ Python.h typeslots.h Python/ abstract.h ... internal/ pycore_atomic.h ... End users/distutils should only -Iinclude, while our build can -Iinclude/Python and -Iinclude/internal as well. Or we can update all of our includes to specify the directory (which I'd prefer - #include "name.h" should look adjacent to the file first, while #include looks at search paths first, which should be able to deal with collisions). Any change here is an improvement, though. I'll be happy for Py_BUILD_CORE to go away (or at least only imply -Iinclude/internal, rather than actual preprocessor work), no matter where files land precisely :) Cheers, Steve On 31Oct2018 1835, Eric Snow wrote:

...

On the one hand dropping redundancy in the filename is fine. On the other having the names mirror the public header files is valuable. How about leaving the base names alone and change the directory to "pyinternal"?

-eric

On Wed, Oct 31, 2018, 17:36 Victor Stinner mailto:vstinner@redhat.com wrote:

Ok, thanks. I decided to remove the redundant "py", so I renamed "pystate.h" to "pycore_state.h" (single "py", instead of "pycore_pystate.h", double "py py").

I updated my PR: https://github.com/python/cpython/pull/10263

* Rename Include/internal/ header files:

* pyatomic.h -> pycore_atomic.h * ceval.h -> pycore_ceval.h * condvar.h -> pycore_condvar.h * context.h -> pycore_context.h * pygetopt.h -> pycore_getopt.h * gil.h -> pycore_gil.h * hamt.h -> pycore_hamt.h * hash.h -> pycore_hash.h * mem.h -> pycore_mem.h * pystate.h -> pycore_state.h * warnings.h -> pycore_warnings.h

* PCbuild project, Makefile.pre.in http://Makefile.pre.in, Modules/Setup: add the Include/internal/ directory to the search paths of header files. * Update include. For example, replace #include "internal/mem.h" with #include "pycore_mem.h".

Victor Le mer. 31 oct. 2018 à 23:38, Eric Snow mailto:ericsnowcurrently@gmail.com> a écrit : > > B > On Wed, Oct 31, 2018 at 4:28 PM Victor Stinner mailto:vstinner@redhat.com> wrote: > > > > Le mer. 31 oct. 2018 à 22:19, Eric Snow mailto:ericsnowcurrently@gmail.com> a écrit : > > > > Maybe we can keep "Include/internal/" directory name, but add > > > > "pycore_" rather than "internal_" to header filenames? > > > > > > this sounds good to me. thanks for chasing this down. > > > > What do you prefer? > > > > (A Include/internal/internal_pystate.h > > (B) Include/internal/pycore_pystate.h > > (C) Include/pycore/pycore_pystate.h > > > > Victor

Victor Stinner

6:19 p.m.

Le jeu. 1 nov. 2018 à 16:38, Steve Dower a écrit :

...

I assume the redundancy was there to handle the naming collision problem, but a better way to do that is to have only header files that users should ever use in "include/", and put the rest inside "include/python/" and "include/internal/" (and if possible, we don't distribute the internal directory as part of -develop packages, which only really impacts the Windows installer as far as we're concerned, but I assume some distros will care).

Aha, I didn't try this approach. I counted 41 Include/ header files which are not included by Python.h: Python-ast.h abstract.h asdl.h ast.h bitset.h bytes_methods.h bytesobject.h code.h codecs.h datetime.h dictobject.h dynamic_annotations.h errcode.h fileobject.h frameobject.h graminit.h grammar.h longintrepr.h marshal.h metagrammar.h node.h opcode.h osdefs.h parsetok.h patchlevel.h pgen.h pgenheaders.h py_curses.h pydtrace.h pyexpat.h pystrhex.h pythonrun.h pythread.h pytime.h setobject.h structmember.h symtable.h token.h ucnhash.h unicodeobject.h For Py_BUILD_CORE, there is at least datetime.h which may be a Include/internal/ twin header. Now I'm not sure how it's going to fit with the second step, "Move !Py_LIMITED_API to Include/pycapi/": https://bugs.python.org/issue35134 Victor

1997

Age (days ago)

2006

Last active (days ago)

List overview

Download

25 comments

6 participants

participants (6)

Benjamin Peterson
Eric Snow
Michael Felt
Neil Schemenauer
Steve Dower
Victor Stinner

Rename Include/internals/ to Include/pycore/

tags

participants (6)