C standard for public headers #30
Comments
|
The C API is now tested by test_cext and test_cppext: these tests checks that including test_cext has 5 tests using
test_cppext has 4 tests using
Without extra compiler flags, we're good. The problem of the issue gh-120293 are the usage of |
|
This issue is not about the tests, but about the C standard we want the headers to support. Again, the tests currently pass even though the headers use a feature that's not in the C99 standard. If the intent is to follow the standard, then the test should change. |
Well, "standard" is one thing, "what's being used in practice" is something else. No C compiler respect the standard by default, they add "compiler extensions", and require special compiler options to respect the standard. For example, unnamed unions are supposed by all C compilers used by Python (GCC, clang, MSC) in C99 mode. So the question is if we care about "strict standard" or "what's being used in practice by C compilers supported by Python" (so including compiler extensions). In practice, the question is if we want to support I proposed to not support |
|
So, you're saying that we should only try to be compatible with specific compilers (and compiler options), rather than C standards?
I don't think supporting C89 is on the table, so static inline functions aren't really relevant. |
Yes.
I was referring to the fact that static inline function bodies expose a lot of code and usually compiler problems come from such code. For example, of pyatomic functions would be regular opaque functions, we wouldn't have this discussion about |
|
Well, I don't see why you brought up Anyway, I think I understand your opinion now; I just disagree with it. I guess it's time for others to chime in. |
|
I am confused though. Can you cleanly summarize the two opinions on the table? |
With c99: With gnu99: With c11:
This has nothing to do with "compiler extensions". It is about the fact that some compilers, like GCC, will allow any code that is legal by a later standard to also compile with a newer standard if the alternative is an error. The theory is that "well you know, your meaning is obvious, you wrote code that is legal in c23 but not c17, so clearly you meant to build with c17". This doesn't actually mean it's safe to ignore the fact that it compiles by default. You can get the same issue if you upgrade to future gcc 15, and start using c23 features. As long as there isn't an incompatible alternative meaning in earlier standards, the compiler typically accepts it. Hopefully with a warning that you're doing something which isn't ISO C99. ... which brings us back to using -Werror=pedantic, since that offers a chance to catch cases where you're using code that isn't legal for a std but the compiler is accepting it anyways "to be nice". If it was a compiler extension it would be allowed when compiling with "GNU C", the collection of GCC vendor extensions on top of standard C. But it is not. |
If I understand correctly, the options are:
Option 2 requires mandating specific compilers, and also specific compiler versions (e.g. require that GCC is at least "whichever version of GCC first implemented c11 unnamed unions"). |
The problem is not specific to So it seems like C11 is what we need/want here. |
I suggest to require C11 and C++03 at minimum to use the Python C API. |
|
@guido, sorry for the delay.
1. Document the standards we support.The C API guidelines will list the C/C++ standards that
We'd welcome PRs to improve conformance, and to make the tests better in verifying conformance. (Within reason of course -- the guidelines can always be changed.) 2. Don't.We support specific compilers, as listed in PEP 11. The supported compiler options are defined in the tests/CI. (That's my understanding; I'll let Victor clarify.) |
That's not my intent. I mean that in practice, we can support C99 with compiler extensions, such as |
Anyway, as I wrote previously, I agree with Petr and I suggest to require C11 to get unnamed unions and other nice C11 features. |
I don't think we can reasonably require it in our headers (any of the headers that we distribute, "internal" or not), until the build backends used by package publishers have released stable versions that automatically add in the compiler options necessary to enable these modes. This isn't a decision that affects us - it affects our users, who will suddenly find that they cannot compile with our newest releases due to code that they can't change. I'd prefer to jump through as many hoops as possible to avoid breaking our users like that. I think it's worth the effort to not use these features in public header files (again, whether "internal" or not) so that we don't put the burden on our users. As far as I'm aware, nothing is broken for us, we just have to do a little bit more typing in our own code. |
My attempt to fix the issue for PyCode and PyOptimizer APIs: python/cpython#120643 |
|
@encukou: I suggest to declare that the Python C API should respect ISO C99.
I merged my PR, so the Python C API is basically compatible with strict ISO C99. Only Free Threading build has one exception: PyObject uses pragma/extension for its unnamed union. |
|
By the way, I also fixed the "const qualifier" warnings: python/cpython#120593 |
|
I created a PR to updated PEP 7: python/peps#3862
|
|
A clarification: the anonymous union has been there since Python 3.12 and it's part of the default (not free-threaded) build's definition of |
|
To add on to Sam's clarification, here's the two line anonymous union: #if (defined(__GNUC__) || defined(__clang__)) \
&& !(defined __STDC_VERSION__ && __STDC_VERSION__ >= 201112L)
// On C99 and older, anonymous union is a GCC and clang extension
__extension__
#endif
#ifdef _MSC_VER
// Ignore MSC warning C4201: "nonstandard extension used:
// nameless struct/union"
__pragma(warning(push))
__pragma(warning(disable: 4201))
#endif
union {
Py_ssize_t ob_refcnt;
#if SIZEOF_VOID_P > 4
PY_UINT32_T ob_refcnt_split[2];
#endif
};
#ifdef _MSC_VER
__pragma(warning(pop))
#endifIn any case where we don't have the back-compat constraints that we had here, it's definitely going to be less code to just avoid a nameless union. |
|
Heh, no wonder why compiling with Here's my suggestion for the PEP update: * The public C API should be compatible with C99 and with C++03.
The existing API does use a few C11 features which are
commonly available as compiler extensions to C99.
New API should not use these features. |
I updated my PEP PR with these changes: python/peps#3862 |
|
I proposed a different PR to require C11 and C++11: python/peps#3896 |
|
This was partially covered in a PR in
This:
As I see it, the remaining open question is whether we should drop C89/C99. The latest public discussion around this, from a year ago, suggests that we shouldn't. |
|
I would prefer to just say:
We can still attempt to remain compatible with C89 and C99 on specific cases, like some compiler options. But IMO it's time to move on and say that C11 is needed. |
I think we'd need to tie this change to a specific release, and make it fairly public, particularly so that build backends have a chance to update their default settings for that version and to warn their users that their code's behaviour may change. |
|
The C standard question arose in issue python/cpython#123747 where I suggest to draw a line and require C11 to use the Python C API in Python 3.14. |
|
IMO, internal code is already C11+. Reverting to C99 there is a friendly gesture for Cython as they work to remove use of private APIs. And it seems making the public headers C11+ would not be friendly to Cython -- according to that issue, they want C99. |
|
I read again the discussion. While C11 would solve most discussions, we don't want to require C11 right now. I understood that C99 would be preferred in practice. The problem is that the C API is not compatible with strict ("pedantic") C99. So I propose:
|
|
Can we also add "warning free under |
I suggest to check warnings using test_cext and test_cppext. Currently on Windows, test_cext uses |
|
IMO, we should use the same warnings flags for all of the code, not just the public API. |
Yes, but that's fairly aspirational. We have a range of other tests that ensure that our code works, despite But if we trigger warnings in code that's included in other people's projects, we impact their ability to get clean. So it's not that we're checking our own API with tighter flags, but we're allowing our users to check their own code with them without having to suppress our own warnings. |
|
Makes sense. Still, new compiler versions can add new warnings, so it's probably best to check in this tests, and not make a guarantee for users. |
|
We do check in tests now :) Victor was right on it |
|
So what do you think of requiring "C99 with compiler extensions for atomic variables and unnamed unions": #30 (comment) ? |
|
Provided they don't raise any warnings, they're fine. (And provided we're open to adding more build configurations to test for warnings, if users need them, though I expect that /W4 covers enough, and the |
|
OK, let's treat it as a change from what's now in the pre-PEP. Rather than “Unnamed unions”, let's refer to the feature as anonymous structures and unions. (I don't think there is a compiler that supports anonymous unions but not structs.) By requiring anything from C11, we break C99Features already needed by Python 3.6:
Not in C++03:
Made optional in C11:
Not interesting for API:
C11Proposed here:
Might be useful:
Not in C++:
Not necessary (can be #defined to no-op):
Not interesting for API:
Optional, not considered:
|
|
Anonymous unions would be quite useful for keeping API compatibility. We already have one in 3.14. Let's allow them. Proposal: Publicly, we simply say that the public headers should be usable with both of C11 and C++11. Internally, we add few features individually, as needed, each with some care when it's first introduced to public headers:
Let's wrap any new keywords in macros (e.g. Does that sound good? |
Add "warning free on max settings" here and I'm down. |
|
I'd rather put that in tests, or note it in the devguide:
We already have “No compiler warnings with major compilers (gcc, VC++, a few others).” in PEP 7. IMO, it also would be great to test something like MSVC “max settings” C11, or C89 with specific errors like C4201 ignored. (Could you add that? You probably know the necessary incantations best. If it fails currently, delegate the actual fixes -- skip the test and leave an issue open.) |
It's more a promise that we'll fix it (and a reminder to whoever is editing headers right now that they've got to suppress warnings they may introduce). And Victor has added tests already.
Deliberately, yeah. For MSVC I'd mean |
For the purpose of winning arguments, is there anything in “Max settings” that “C11 and C++11” doesn't catch? “Max settings” makes sense in this conversation, but I think the public version would need to be much longer. Or do you have a reasonable wording in mind?
Not the |
But I do mean options like I'm not massively concerned about eagerly going through and getting warning clean - if we state a policy then people will do that for us 😉 - but we should decide in advance what our response will be if someone comes to us asking to prevent that warning. As for wording, I think we can say something like "most strict warning preset offered by the compiler, such as |
|
I disagree here: I think that if someone asks for But either way, I think this is out of scope for this issue (“C standard for public headers”). Would you be OK doing it in a future step, or do you think “both of C11 and C++11” is unacceptable? |
They can't fix it, because we've already built with padding, and so the structs have to reflect reality at the point where a consumer is using our headers. Anything that would require edits to our code (including our headers) should be suppressed, and we shouldn't force users to figure that out themselves (how many places are they going to
I think it's too easy to [mis]interpret in ways that people can use to justify making changes to the headers that will break users. We aren't requiring those language specs literally, we're requiring the feature sets, and if default compiler extensions to C99 or C++03 make things work (as they currently do) then they should be allowed to continue working. If we want to get to those language versions specifically (i.e. require |
|
OK, I now see where we disagree. However, if the user requests additional warnings, that's a different situation in my eyes. I don't think we should be silently overriding that preference. Well, I think we're out of scope of a “decision”. Do you want to start a Discourse discussion for “max settings”? I might not be able to word your proposal well.
We have tests for that.
Ideally no edit would be needed, you should be OK with a pragma push/pop, marking code where you don't want the additional diagnostics.
If you ask me: ideally, one :) |
The problem is that they'll be requesting them for their own code, but due to how includes work, our code inevitably is covered by that. But they don't want to know about issues in our code, just their own. That's why ours should override those preferences (in places where we've already seen the warning ourselves and verified that we shouldn't just fix our own code, which is why I'm not keen on the idea of broad suppressions for all of our headers, even though I suspect that's inevitably where we'll end up).
So the public docs will say "our headers don't use anything later than C11 or C++11, and if your compiler has a mode for these language versions you don't have to enable it", and our contribution guidelines will list the exact features we're allowed to use? That works for me, but I'm not sure it's what you're proposing.
Provided we write the push/pop into our own code, yes, that's fine. We shouldn't make our users write that themselves around our header.
Well, guess who's going to be disappointed then 😉 |
Yes, that's what I meant for the vote. The “max settings” conversation is too interesting; can we split it out of here? |
|
Sure. I don't have a particular place to split it to, though. Is it something we need to decide (and put... where?) |
|
Thanks for the vote, everyone! I opened a CPython issue python/cpython#129666.
I think Discourse would be best. Silencing all known warnings probably will be more controversial (at least I hope so); IMO we should talk it out in public. |
Go ahead. Just present it as "avoiding all known warnings" rather than "silencing" 😉 The point is to write reliable, portable code that can be used in a wide variety of circumstances, not merely to shut up the complaints. |
Previous discussion: capi-workgroup/problems#42 & capi-workgroup/api-evolution#22
It seems the consensus of the people involved is that
Python.hshould be usable with:Now that there's a report about the 3.13 headers containing a C11 feature (anonymous structs/unions), I guess it's time to make an official guideline.
The text was updated successfully, but these errors were encountered: