This implements N2393.
I am a bit confused by the proposal only taking about adding "true" and "false" constants but then still using "bool" as a type. I'm not sure if this part of the patch is 100% correct.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| clang/test/Sema/c2x-bool.c | ||
|---|---|---|
| 10 | Not that clang-format wanted me to format these two lines, but the result was pretty ugly: -_Static_assert(_Generic(true, bool : true, default: false)); -_Static_assert(_Generic(false, bool : true, default: false)); +_Static_assert(_Generic(true, bool + : true, default + : false)); +_Static_assert(_Generic(false, bool + : true, default + : false)); | |
Comment Actions
Thanks for working on this! It's worth noting that http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2935.pdf was what was adopted by WG14 a few weeks ago, so you should be checking against that one, not the initial version of the paper.
It's also worth noting that your changes are partially covering http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2934.pdf which was adopted at the same meeting (setting Opts.Bool to true means you get bool and true and false); I would note that in the commit message, because I don't think it's worth trying to separate out bool, true, and false from one another in the implementation. But it is worth adding the deprecation warning to use of _Bool from that paper and removing the macro for bool in C2x mode from <stdbool.h> as part of these changes in this patch.
I think you're missing some test coverage for the preprocessor in the patch. e.g.,
#if true != 1 #error "oops" #endif #if false != 0 #error "oops" #endif
And I don't see the changes to the <stdbool.h> header which is now obsolete in C2x.
Oh, and the change should be added to the release notes.
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 3247 | Comment is now stale. | |
| clang/test/Sema/c2x-bool.c | ||
| 1 | Is the triple necessary? | |
| 14 | You should also add a test to verify that the predefined constants are usable as an integer constant expression: char c1[true]; // good _Static_assert(sizeof(c1) == sizeof(char[1])); char c2[false]; // zero-sized array declaration (This covers the changes in 6.6.) | |
| 15 | Another test case that's needed is for the integer promotion rules: _Static_assert(_Generic(+true, bool : 0, unsigned int : 0, signed int : 1, default : 0));
struct S {
bool b : 1;
} s;
_Static_assert(_Generic(+s.b, bool : 0, unsigned int : 0, signed int : 1, default : 0)); | |
Comment Actions
Thanks for the links to the papers. What's the best way of updating https://clang.llvm.org/c_status.html#c2x with the new papers?
I added the changes to stdbool.h, but I haven't been able to come up with a good way of testing them...
| clang/docs/ReleaseNotes.rst | ||
|---|---|---|
| 113 ↗ | (On Diff #413279) | This line is from https://reviews.llvm.org/D119221 and probably why the patch application fails. Not relevant for review I think but I'll fix it up next time. |
Comment Actions
I update that page from the WG14 editor's report (or the latest working draft) so that I can be sure we're tracking what actually made it into the standard. I'll try to get it right when I add these papers to the list, but you may want to keep your eyes peeled when I update the page to double-check just in case.
I added the changes to stdbool.h, but I haven't been able to come up with a good way of testing them...
I think stdbool.h should get a deprecation warning, and that will make it testable. e.g., https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdnoreturn.h#L16
| clang/lib/Headers/stdbool.h | ||
|---|---|---|
| 13–14 ↗ | (On Diff #413279) | FWIW, I've been using __STDC_VERSION__ > 201710L for my C2x predicates. My plan was to switch them to use the C2x __STDC_VERSION__ value once that's been finalized. Either approach works though. |
| 16 ↗ | (On Diff #413279) | We're inside an #if __STDC_VERSION__ check here, so I think this will now always evaluate to true. |
| 23–28 ↗ | (On Diff #413279) | This seems wrong now too. |
| clang/lib/Headers/stdbool.h | ||
|---|---|---|
| 16 ↗ | (On Diff #413279) | Not being able to indent preprocessor directives at all makes this pretty hard to read, but it should work like this. |
Comment Actions
Precommit CI is failing:
Failed Tests (1):
Clang :: Headers/stdbool.c
| clang/lib/Headers/stdbool.h | ||
|---|---|---|
| 18 ↗ | (On Diff #413782) | I'd go with a slightly shorter form of this; the noreturn one was a bit weird because there were multiple alternatives you could use (_Noreturn or [[noreturn]]), but in this case, it's just "don't include this header and everything will be fine". |
| clang/test/Headers/stdbool.c | ||
| 2 ↗ | (On Diff #413782) | If you're playing along at home, stdbool.cpp tests the C++ behavior, so we don't need an additional RUN line here for that. However, I'd appreciate a RUN line that defines _CLANG_DISABLE_CRT_DEPRECATION_WARNINGS to demonstrate we don't emit the warning or the defines in that case. |
Comment Actions
Heh, I guess I need to make a habit of running the tests also after clang-format :/
| clang/test/Headers/stdbool.c | ||
|---|---|---|
| 2 ↗ | (On Diff #413782) | I don't understand, both of the RUN lines are for C. |
Comment Actions
LGTM!
| clang/test/Headers/stdbool.c | ||
|---|---|---|
| 2 ↗ | (On Diff #413782) |
An earlier form of your patch broke the behavior in C++ and I didn't recall if the precommit CI pipeline caught the failure or not. So I thought "hmm, should we have a C++ RUN line here to catch that break or do we already have test coverage?" It turns out we have existing coverage, so I was reporting that so other reviewers (including drive-by reviews) knew we already thought about that. Thanks for adding the other RUN line for the macro definition! |
Comment Actions
We've started having several internal user complaints because their system headers are suddenly triggering a warning for using <stdbool.h>. This seems to be caused by the fact that #warning is surfaced to users even when the #warning is present in a system header (which makes sense, because otherwise there would be no way to issue a #warning from a system header). This ends up causing problems because users have no way to suppress the warning in the system headers they use without also disabling deprecation warnings in their own code. Is this intended? Instead, it seems to me like what we'd want is some way to mark the header as deprecated such that Clang will not flag uses of <stdbool.h> from within system headers, but will flag them from user code. This would be consistent with how the deprecated attributes work for classes and functions.
Thoughts?
Comment Actions
This is what _CLANG_DISABLE_CRT_DEPRECATION_WARNINGS is for. We documented it here: https://clang.llvm.org/docs/UsersManual.html#controlling-deprecation-diagnostics-in-clang-provided-c-runtime-headers but the basic idea is, if that's defined before including the header, we don't issue those warnings. Does that suffice?
Comment Actions
Well, the problem is that a user that is using some system header like Foundation.h (which includes <stdbool.h>) will be forced to define _CLANG_DISABLE_CRT_DEPRECATION_WARNINGS, which has two drawbacks:
- It's annoying to them because they never used <stdbool.h> in their own code
- It turns off deprecation warnings in their own code as well
In other words, the fact that this #warning gets out of system headers means that it's going to "spam" users who have no control over what their system headers are doing. So while the long-long term fix is for all system headers (like Foundation.h) to be C2x friendly and not include <stdbool.h> in newer standard modes, the current migration path involves a lot of spammy warnings for end users in the meantime. Since such a migration may or may not happen completely everywhere, it also means that this is probably not a "bite the bullet for 3 months" situation.
FWIW, although libc++ is fairly aggressive on marking functions and classes as deprecated, we do not try to warn for *headers* that are deprecated, because there is no satisfying way of doing so right now. Personally, my opinion is that unless we have a way to mark headers as deprecated in a way that the compiler can understand the semantics and have it behave roughly like a "deprecated attribute but on a header" (which means that this warning would behave just like all other compiler warnings w.r.t. system headers), it is not a great idea to do this. I'm very keen on deprecating stuff, however I think this #warning is not the right tool for it, because of the downsides I mentioned.
Comment Actions
The system header including a header that's explicitly deprecated is dubious practice, but that does still require some amount of timing coordination.
In other words, the fact that this #warning gets out of system headers means that it's going to "spam" users who have no control over what their system headers are doing. So while the long-long term fix is for all system headers (like Foundation.h) to be C2x friendly and not include <stdbool.h> in newer standard modes, the current migration path involves a lot of spammy warnings for end users in the meantime. Since such a migration may or may not happen completely everywhere, it also means that this is probably not a "bite the bullet for 3 months" situation.
Okay, that's a fair problem. However, the flip side is: why should C2x users not be told about deprecated functionality when they include the header directly?
FWIW, although libc++ is fairly aggressive on marking functions and classes as deprecated, we do not try to warn for *headers* that are deprecated, because there is no satisfying way of doing so right now. Personally, my opinion is that unless we have a way to mark headers as deprecated in a way that the compiler can understand the semantics and have it behave roughly like a "deprecated attribute but on a header" (which means that this warning would behave just like all other compiler warnings w.r.t. system headers), it is not a great idea to do this. I'm very keen on deprecating stuff, however I think this #warning is not the right tool for it, because of the downsides I mentioned.
So I take it https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdnoreturn.h is also an issue?
My feeling is: system headers that spam warnings are a bigger concern than not getting a deprecation warning, but we want that deprecation warning eventually because we need *some* way to deprecate headers as a whole. Not issuing deprecation warnings means standards bodies can't reasonably rely on users having been warned, so not diagnosing means "let's stick the *entire ecosystem* with this header file forever". So I'm thinking of walking back the #warning for the headers but with some sort of timeline as to when we can add the diagnostics back to the headers. Do you have an idea of what would be reasonable for Foundation.h?
We could maybe extend #pragma clang deprecate to deprecate inclusion of the current file so that it acts sort of like a [[deprecated]] attribute that triggers on inclusion using typical diagnostics instead of #warning.
Comment Actions
The tricky part to this is the pragma would effectively have to work like "diagnose on exit from this file when popping the lexing stack" because the header marks itself as being deprecated, so the compiler wouldn't KNOW to warn on the inclusion of the file (since it won't have seen the pragma by that point). It might be tricky to get the correct location for the diagnostic.
Comment Actions
I agree, and I'll be filing bug reports against system headers within my control that use <stdbool.h>. That is unfortunately not the majority of headers, though.
In other words, the fact that this #warning gets out of system headers means that it's going to "spam" users who have no control over what their system headers are doing. So while the long-long term fix is for all system headers (like Foundation.h) to be C2x friendly and not include <stdbool.h> in newer standard modes, the current migration path involves a lot of spammy warnings for end users in the meantime. Since such a migration may or may not happen completely everywhere, it also means that this is probably not a "bite the bullet for 3 months" situation.
Okay, that's a fair problem. However, the flip side is: why should C2x users not be told about deprecated functionality when they include the header directly?
I agree -- they should be told about the deprecated functionality. The problem is that with the currently available technology, we have to choose between telling them about their use of deprecated <stdbool.h> AND spamming them for system headers' use of it, or not telling them at all. Both options have downsides, I think it's a matter of what we value the most.
So I take it https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdnoreturn.h is also an issue?
I think so. I suspect it may be much less widely used, so that might be why it wasn't brought up.
My feeling is: system headers that spam warnings are a bigger concern than not getting a deprecation warning, but we want that deprecation warning eventually because we need *some* way to deprecate headers as a whole. Not issuing deprecation warnings means standards bodies can't reasonably rely on users having been warned, so not diagnosing means "let's stick the *entire ecosystem* with this header file forever". So I'm thinking of walking back the #warning for the headers but with some sort of timeline as to when we can add the diagnostics back to the headers. Do you have an idea of what would be reasonable for Foundation.h?
I fully agree -- we need a way to deprecate headers (just like we do for classes and functions). My intent is really not to impair your efforts at doing that -- I'm just trying to point out the unfortunate tradeoff we are making. Regarding Foundation.h -- I am using this as an example, but it's really just the tip of the iceberg. We can probably get this one fixed within a reasonable time frame, however that won't change the fundamental issue.
We could maybe extend #pragma clang deprecate to deprecate inclusion of the current file so that it acts sort of like a [[deprecated]] attribute that triggers on inclusion using typical diagnostics instead of #warning.
Yes, if that is feasible, I think something like that is exactly what we would need. If we had a tool like this, by the way, I would be very keen on deprecating some libc++ headers that we keep around like <ciso646> & friends.
So, in summary, my opinion is that we should wait until we have the appropriate technology (something like proposed above) before deprecating these headers. And once we do, then I'll be 100% supportive of efforts in that direction. But I'm mostly relaying user feedback here, I don't own the Clang headers so I must defer to your judgement.
Comment Actions
Thanks!
My feeling is: system headers that spam warnings are a bigger concern than not getting a deprecation warning, but we want that deprecation warning eventually because we need *some* way to deprecate headers as a whole. Not issuing deprecation warnings means standards bodies can't reasonably rely on users having been warned, so not diagnosing means "let's stick the *entire ecosystem* with this header file forever". So I'm thinking of walking back the #warning for the headers but with some sort of timeline as to when we can add the diagnostics back to the headers. Do you have an idea of what would be reasonable for Foundation.h?
I fully agree -- we need a way to deprecate headers (just like we do for classes and functions). My intent is really not to impair your efforts at doing that -- I'm just trying to point out the unfortunate tradeoff we are making. Regarding Foundation.h -- I am using this as an example, but it's really just the tip of the iceberg. We can probably get this one fixed within a reasonable time frame, however that won't change the fundamental issue.
Agreed, the issue is that #warning is effectively useless in system headers. :-(
We could maybe extend #pragma clang deprecate to deprecate inclusion of the current file so that it acts sort of like a [[deprecated]] attribute that triggers on inclusion using typical diagnostics instead of #warning.
Yes, if that is feasible, I think something like that is exactly what we would need. If we had a tool like this, by the way, I would be very keen on deprecating some libc++ headers that we keep around like <ciso646> & friends.
I'll explore just how possible this is. There's all sorts of neat problems with it, like header guards/#pragma once meaning the header file's contents aren't scanned on a second inclusion (so we need to keep a list of already-included files that are deprecated), and truly awful things like:
// Foo.h, which has no header guard because it's like assert.h #if WHATEVER ... #else #pragma clang deprecate_header #endif // main.c #define WHATEVER #include "Foo.h" #undef WHATEVER #include "Foo.h" #define WHATEVER #include "Foo.h" #endif
and so on.
So, in summary, my opinion is that we should wait until we have the appropriate technology (something like proposed above) before deprecating these headers. And once we do, then I'll be 100% supportive of efforts in that direction. But I'm mostly relaying user feedback here, I don't own the Clang headers so I must defer to your judgement.
The feedback from your users (and you) is very much appreciated; this is exactly the kind of feedback I hoped to get by doing those changes early in the release (so I still have time to address issues like this before it's too late). So thank you for bringing this up and the discussion around it!
I'll roll back the #warning use and report back when that's done.
Comment Actions
Thank you! I've filed a bug report about Foundation to let them know about their usage of <stdbool.h> which is deprecated in C2x.
Comment is now stale.