Skip to content

Move #include directives before extern "C" { in internal headers #115105

Description

@colesbury

Feature or enhancement

Some of our internal headers (e.g., pycore_ceval.h) contain #include directives inside the extern "C" { blocks. This can cause problems if system headers are included within an extern "C" { block and the header is compiled as C++ code. For a similar issue with the public headers, see #110845.

As far as I can tell, nobody has complained about this yet, but I think we might as well address this sooner rather later.

cc @ericsnowcurrently @gpshead @vstinner

Activity

vstinner commented on Feb 7, 2024

@vstinner
Member

Yes, it sounds like a good idea.

It took me a while, but I think that I understood the C++20 module issue: #110845 (comment)

encukou commented on Feb 7, 2024

@encukou
Member

If someone asks, let's make them happy, but I don't think we need to do this proactively.
Internal headers should be compiled as C.

vstinner commented on May 20, 2024

@vstinner
Member

If someone asks, let's make them happy, but I don't think we need to do this proactively. Internal headers should be compiled as C.

I suggest to close this issue in this case.

colesbury commented on May 20, 2024

@colesbury
ContributorAuthor

@albanD asked me about this a week or two ago in the context of PyTorch. I'm not sure if it's still an issue or if it's been worked around.

encukou commented on May 21, 2024

@encukou
Member

@albanD, please speak if this is an issue for you.
I'll close this in a month if no one complains.

jansel commented on May 21, 2024

@jansel

In TorchDynamo eval_frame.c was implemented in C (as opposed to C++ like the rest of PyTorch), because CPython headers (internal/pycore_frame.h, internal/pycore_pystate.h, etc) did not work from C++.

This was a few years ago at this point, so I forget the exact error -- but the workaround was to implement that one file in C. We would prefer for that file to be implemented in C++.

albanD commented on May 21, 2024

@albanD
Contributor

Thanks for considering our request.
On top of what @jansel mentioned above, I would add:

As you might imagine, this is fairly brittle (very dependent on cpython core include changes) and prevents us from using c++ as with the rest of our codebase.

vstinner commented on May 22, 2024

@vstinner
Member

Some definitions we need for subtype dealloc use, we end up just putting the "extern" in our code to not have to deal with including the corresponding header (pycore_weakref in this case): https://github.com/pytorch/pytorch/blob/1cc9354cb0a1ac2dcd63c73b37f5bec0dec0cba3/torch/csrc/utils/python_compat.h#L36-L37

For _PyWeakref_ClearRef, see capi-workgroup/decisions#25

encukou commented on Jun 21, 2024

@encukou
Member

Is there anything that should be exposed as public (potentially unstable) API, that we could document, test and support?

If you want the compiled code to match CPython, you will want to compile it as C. Especially if you've already taught your build system to do that. I don't think the extern "C" { itself is a major problem -- though it's likely to be the first issue in the file.

From a quick look at the code, it seems that you're reaching into the kind internals that change as CPython is optimized. I'm afraid I don't have advice for you other than: think about what could be contributed to CPython, and what could be put behind a supported API boundary.
(Also: if you're copying code from CPython, please mind the licence.)

I will close this issue. If you must use internal headers, then I think compiling them as C is the way to go. Discussion about bigger issues than moving extern "C" { would be better on Discourse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions