bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Investigate C11 _Static_assert compiler support, and make MOZ_STATIC_ASSERT expand to it when possible to do so without warnings

NEW
Assigned to

Status

()

Core
MFBT
--
minor
7 years ago
4 years ago

People

(Reporter: Waldo, Assigned: Emma Benoit, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
C++11 includes static_assert(cond, reason) syntax to implement static assertions.  MOZ_STATIC_ASSERT expands to this in compilers that support it.  C11 (the next edition of C) includes functionally identical _Static_assert syntax.  Someone (not me) should investigate compiler support for _Static_assert -- particularly for when it can be used without triggering a warning (note that in my cursory investigation clang's docs may be wrong about it triggering a warning except in -std=c11 code, not sure if I'm misreading or either docs or code have a bug) -- and make MOZ_STATIC_ASSERT expand to it when possible.
(Reporter)

Updated

7 years ago
Whiteboard: [good first bug][mentor=Waldo][lang=c]

Comment 1

6 years ago
Would like to take this up

Comment 2

5 years ago
After a bit of research, it looks like GCC and Clang support the _Static_assert macro, but MSVC does not. I have yet to test it in any compiler, so this is simply the result of an hour or so of searching.

Comment 3

5 years ago
I've gotten _Static_assert to run on Clang, but as I suspected, it does not appear to work in MSVC. GCC quits on a "not defined in this scope" error for both _Static_assert and static_assert. I suspect that this is due to a faulty GCC setup on Windows. I'm nearly a total newbie, so that's entirely likely.

Additionally, could I be marked as the assignee of this bug?
(Reporter)

Updated

5 years ago
Assignee: nobody → briguy92
Status: NEW → ASSIGNED

Comment 4

5 years ago
Created attachment 733566 [details] [diff] [review]
Patch to use _Static_assert in clang

I haven't used Mercurial before, so I'm not sure if I created this patch correctly. Please let me know if it's messed up in any way.
(Reporter)

Comment 5

5 years ago
I'll take a look at this shortly.
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 6

5 years ago
Hmm, this patch is indeed a bit weird.  It looks like it applies to a file that has _Static_assert uses in it already.  And it's reverting __GNUC__-conditioned _Static_assert uses to the current system for that compiler, that doesn't use _Static_assert.  Maybe this is a diff against a patch you'd created, and not against an unmodified tree?  I'd suggest asking on irc.mozilla.org in the #introduction channel for Mercurial help, to see if you can get realtime help creating a fully-properly-generated patch.
Flags: needinfo?(jwalden+bmo)

Comment 7

5 years ago
Brian, could you confirm that you're still working on this?
Flags: needinfo?(briguy92)

Comment 8

5 years ago
Yes, I am still working on this. I haven't had a chance to look at it in a while due to school, but will upload a new patch as soon as I can get my Mercurial setup figured out.
Flags: needinfo?(briguy92)

Comment 9

4 years ago
OK, I think that I need to throw this one back in the pool.
Assignee: briguy92 → nobody
Status: ASSIGNED → NEW
Mentor: jwalden+bmo
Whiteboard: [good first bug][mentor=Waldo][lang=c] → [good first bug][lang=c]
(Assignee)

Comment 10

4 years ago
Created attachment 8443926 [details] [diff] [review]
Edited mtbf/Assertions.h to expand MOZ_STATIC_ASSERT to _Static_assert or static_assert with clang, gcc and MSC
Attachment #8443926 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Assignee: nobody → kylma
(Reporter)

Comment 11

4 years ago
Comment on attachment 8443926 [details] [diff] [review]
Edited mtbf/Assertions.h to expand MOZ_STATIC_ASSERT to _Static_assert or static_assert with clang, gcc and MSC

Review of attachment 8443926 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +99,5 @@
> +#    endif
> +#    if __has_extension(c_static_assert)
> +#      define MOZ_STATIC_ASSERT(cond, reason) _Static_assert((cond), reason)
> +#    elif __has_extension(cxx_static_assert)
> +#      define MOZ_STATIC_ASSERT(cond, reason) static_assert((cond), reason)

Given that we're in a !__cplusplus block, I think these two lines can/should be removed.

@@ +108,5 @@
> +#  elif defined(__GNUC__)
> +#    if MOZ_GCC_VERSION_AT_LEAST(4, 6, 0)
> +#      define MOZ_STATIC_ASSERT(cond, reason) _Static_assert((cond), reason)
> +#    elif MOZ_GCC_VERSION_AT_LEAST(4, 3, 0)
> +#      define MOZ_STATIC_ASSERT(cond, reason) static_assert((cond), reason)

This too.

@@ +114,5 @@
> +#      define MOZ_STATIC_ASSERT(cond, reason) \
> +          extern void MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __LINE__)(int arg[(cond) ? 1 : -1]) MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE
> +#    endif
> +#  elif defined(_MSC_VER)
> +#      if MSC_VER >= 1600 /* MSVC 10 */

_MSC_VER typo

@@ +118,5 @@
> +#      if MSC_VER >= 1600 /* MSVC 10 */
> +#         define MOZ_STATIC_ASSERT(cond, reason) static_assert((cond), reason)
> +#      else
> +#          extern void MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __LINE__)(int arg[(cond) ? 1 : -1]) MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE
> +#      endif

This bit was mis-indented -- should be

#  elif defined(_MSC_VER)
#    if _MSC_VER >= 1600 /* MSVC 10 */
#      define MOZ_STATIC_ASSERT(cond, reason), static_assert((cond), reason)
#    else
#      extern void...
#    endif
#  elif defined(__COUNTER__)
Attachment #8443926 - Flags: review?(jwalden+bmo) → feedback+
(Reporter)

Comment 12

4 years ago
Also: now that it's landed, please use MOZ_MSVC_VERSION_AT_LEAST() for the MSVC version check.  So much more readable!
Does MSVC10+ support static_assert in C-mode?
(Assignee)

Comment 14

4 years ago
I will update the patch as soon as I can.

@emk: I found this http://msdn.microsoft.com/en-us/library/bb918086%28v=vs.90%29.aspx which seems to indicate that it does.
(In reply to Emma Benoit from comment #14)
> I will update the patch as soon as I can.
> 
> @emk: I found this
> http://msdn.microsoft.com/en-us/library/bb918086%28v=vs.90%29.aspx which
> seems to indicate that it does.

I couldn't find any explanation from that MSDN article about neither C++99 static_assert nor C11 _Static_assert. The _STATIC_ASSERT macro uses a trick to implement static assertion for older compilers.
(Reporter)

Comment 16

4 years ago
(In reply to Masatoshi Kimura [:emk] from comment #13)
> Does MSVC10+ support static_assert in C-mode?

MSVC10+ doesn't even support C-mode.  :-)  The C support they have is, as a matter of policy, largely incident to their support for C++.  No reason to do anything for MSVC for this bug.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> MSVC10+ doesn't even support C-mode.  :-)

It does. It will compile the source files with .c extension in C-mode. It also has the -TC option to force compile sources in C-mode.

> The C support they have is, as a
> matter of policy, largely incident to their support for C++.  No reason to
> do anything for MSVC for this bug.

MSVC doesn't recognize the inline keyword in C-mode. I suspect the static_keyword is the same.
s/static_keyword/static_assert keyword/;
(Assignee)

Comment 19

4 years ago
Created attachment 8446150 [details] [diff] [review]
mfbt/Assertions.h: expand MOZ_STATIC_ASSERT with clang and gcc

Updated, I removed the MSVC part.
Attachment #733566 - Attachment is obsolete: true
Attachment #8443926 - Attachment is obsolete: true
Attachment #8446150 - Flags: feedback?(jwalden+bmo)
(Reporter)

Comment 20

4 years ago
Comment on attachment 8446150 [details] [diff] [review]
mfbt/Assertions.h: expand MOZ_STATIC_ASSERT with clang and gcc

Review of attachment 8446150 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable.  Make the one change and post for review, and I can land it for you.

::: mfbt/Assertions.h
@@ +100,5 @@
> +#    if __has_extension(c_static_assert)
> +#      define MOZ_STATIC_ASSERT(cond, reason) _Static_assert((cond), reason)
> +#    else
> +#      define MOZ_STATIC_ASSERT(cond, reason) \
> +          extern void MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __LINE__)(int arg[(cond) ? 1 : -1]) MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE

Rather than copy this version, please copy the __COUNTER__ version defined below.  Also use it in the __GNUC__ case as well.  Ideally we'd have this code fall through into the actual __COUNTER__ version below, but unfortunately this

#if  defined(TEST_MACRO) && MACRO_DEFINED_ONLY_IF_TEST_MACRO_DEFINED()

doesn't short-circuit, because of how preprocessing works, so nesting and duplication like this is the most practical solution.
Attachment #8446150 - Flags: feedback?(jwalden+bmo) → feedback+
You need to log in before you can comment on or make changes to this bug.