Differential D17444
[MSVC] Recognize "static_assert" keyword in C mode Authored by rnk on Feb 19 2016, 5:08 AM.
Details MSVC treats static_assert as a keyword in C mode and they implement in This is a non-conforming extension (the keyword is outside the Fixes PR26672 Patch by Andrey Bokhanko!
Diff Detail
Event TimelineComment Actions I think we should do this because MSVC doesn't make _Static_assert available to C code. David says that, according to the C standard, assert.h is supposed to #define static_assert _Static_assert. MSVC doesn't do that because they provide static_assert directly as a keyword. Pretty soon we'll start seeing portable C projects including assert.h to use plain static_assert, and if we don't do something, that code will compile with MSVC but not clang-cl. I think we should do this under fms-compatibility, not fms-extensions, because it is technically not a conforming extension. =/ There isn't a bit for that currently, but we should add one. Maybe call it KEYMSCOMPAT? Comment Actions Do they treat this as a keyword or as a predefined macro? (Does #ifdef think static_assert is defined?) Comment Actions Patch updated in response to Reid's proposal (to introduce KEYMSCOMPAT). (To clarify, I agree with Aaron that we should wait for MS' response first. Though I frankly don't expect them to accept recognition of "static_assert" as a bug, so wanted to have a patch ready.) Yours, AndreySoftware Engineer Comment Actions It seems the discussion (http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160229/thread.html#151860) concluded that we don't want this fix to be committed until we see some compelling code that relies on "static_assert" availability in C language without <assert.h> being included. Thus, please consider this review request to be abandoned. Yours, Comment Actions I just ran into this and I'm a bit confused about the discussion here. This snippet (in a .c file) #include <assert.h> static_assert(4 == 4 , ""); builds in all compilers except clang-cl. How does not supporting this make sense? Instead of this patch we could have an assert.h wrapper in lib/Headers that defines static_assert to _Static_assert in ms mode for C files, and I suppose that's a cleaner fix. But I hope it's not controversial that we should try and support standard C programs? Comment Actions I'd prefer it if we didn't shadow more MSVC CRT headers like assert.h. Users have a long history of holding the compiler wrong, losing these interpositions, and having things not work. If we could just work out the box, users will have less problems, we'll have fewer bug reports, etc, etc. I still think we should follow MSVC and add this is a plain old C++ extension to C under -fms-extensions. Comment Actions Given that we already have interposition headers and users already have to hold the compiler right, does it matter if we have one more interposing header? Comment Actions I'd say it's one more bit of technical debt that we'll have to figure out how to undo later. I'm OK doing it if @rsmith still feels that we shouldn't do this in the compiler. Also, I suppose this is really actually an -fms-compatibility thing not -fms-extensions because it takes an identifier from outside the implementer's namespace. Comment Actions
I just verified, it really does take the identifier and it does not treat static_assert as a macro (even when you include assert.h). I suppose we probably should treat this as a keyword like MSVC seems to be doing, rather than interposing on assert.h, because the interposition would give subtly different resulting behavior. Comment Actions I don't have an opinion if this patch here or the assert wrapper is better, but I feel somewhat strongly that we should land one or the other so that the standard C program #include <assert.h> static_assert(4 == 4 , ""); builds in clang-cl like it does in every other compiler. Sounds like rnk and rsmith are the champions for each approach, so maybe the two of you could have a meeting of the minds somewhere and Decide What To Do Here? Comment Actions I chatted with @rsmith offline and the solution we came up with was to do both of the following:
However, local testing has shown that Visual C++ doesn't recognize _Static_assert, so the second part of this isn't so simple. The problem is that we want users to be able to compile the following conforming C11 code with -fno-ms-compatibility: #include <assert.h> static_assert(true, "foo"); The other solution is to have VC assert.h say: #ifdef __clang__ #define static_assert _Static_assert #endif ... but maybe it would be better to teach the Visual C++ compiler to recognize the C11 _Static_assert keyword in the first place. Comment Actions Awesome, thanks.
| ||||||||||
Should this comment (or a comment elsewhere) mention the plan that we want asked for the change to msvc's assert.h and that we want to stop defining this if _MSC_VER is new enough to have that change?