Page MenuHomePhabricator

[MSVC] Recognize "static_assert" keyword in C mode
ClosedPublic

Authored by rnk on Feb 19 2016, 5:08 AM.

Details

Summary

MSVC treats static_assert as a keyword in C mode and they implement in
the C++11 way. It does not support _Static_assert in C mode either.

This is a non-conforming extension (the keyword is outside the
implementer's namespace), so it is placed under -fms-compatibility
instead of -fms-extensions like most MSVC-specific keyword extensions.

Fixes PR26672

Patch by Andrey Bokhanko!

Diff Detail

Repository
rC Clang

Event Timeline

andreybokhanko retitled this revision from to PR26672: [MSVC] Clang does not recognize "static_assert" keyword in C mode.
andreybokhanko updated this object.
andreybokhanko added a subscriber: cfe-commits.
rnk edited edge metadata.Feb 19 2016, 11:02 AM

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?

rsmith added a subscriber: rsmith.Feb 19 2016, 1:14 PM

Do they treat this as a keyword or as a predefined macro? (Does #ifdef think static_assert is defined?)

andreybokhanko edited edge metadata.

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,

Andrey

Software Engineer
Intel Compiler Team

rnk accepted this revision.Mar 2 2016, 5:44 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 2 2016, 5:44 PM
andreybokhanko abandoned this revision.Mar 9 2016, 7:47 AM

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,
Andrey

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?

rnk added a comment.Feb 11 2019, 10:40 AM

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?

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.

In D17444#1393339, @rnk wrote:

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?

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.

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?

rnk added a comment.Feb 11 2019, 1:08 PM

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?

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.

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.

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.

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?

rnk added a subscriber: STL_MSFT.Feb 13 2019, 2:06 PM

I chatted with @rsmith offline and the solution we came up with was to do both of the following:

  • make static_assert a keyword under -fms-compatibility (it is a technically non-conforming extension)
  • ask @STL_MSFT if #define static_assert _Static_assert can be added to the VC assert.h header. This has the benefit of fixing the issue for users with a new VC and an old clang.
  • when a fixed assert.h is released, remove this nonconforming keyword from clang, or make it conditional on a new enough _MSC_VER

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.

rnk updated this revision to Diff 186741.Feb 13 2019, 2:13 PM
  • rebase
This revision is now accepted and ready to land.Feb 13 2019, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2019, 2:13 PM
rnk commandeered this revision.Feb 13 2019, 2:13 PM
rnk edited reviewers, added: andreybokhanko; removed: rnk.

Comandeering and updating description...

This revision now requires review to proceed.Feb 13 2019, 2:13 PM
rnk retitled this revision from PR26672: [MSVC] Clang does not recognize "static_assert" keyword in C mode to [MSVC] Recognize "static_assert" keyword in C mode.Feb 13 2019, 2:14 PM
rnk edited the summary of this revision. (Show Details)
rnk edited reviewers, added: rsmith; removed: majnemer, andreybokhanko.
rnk edited subscribers, added: andreybokhanko; removed: rsmith.

@rnk I've forwarded this to the compiler front-end and Universal CRT teams.

rnk added a comment.Feb 13 2019, 2:43 PM

@rnk I've forwarded this to the compiler front-end and Universal CRT teams.

Thanks!

thakis accepted this revision.Feb 13 2019, 8:20 PM

Awesome, thanks.

clang/test/Lexer/keywords_test.cpp
57 ↗(On Diff #186741)

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?

This revision is now accepted and ready to land.Feb 13 2019, 8:20 PM
This revision was automatically updated to reflect the committed changes.