Add static assert macro #3113

geky · 2016-10-21T20:06:52Z

Add static assert macro

Adds MBED_STATIC_ASSERT for compile-time assertions, results in compile-time error if condition is false.

Example:

MBED_STATIC_ASSERT(MBED_LIBRARY_VERSION > 120);

Assertion failure on GCC 5.3.1:

test.cpp:3:69: error: size of array is negative
 #define MBED_STATIC_ASSERT(expr) void _static_assert(int[(expr)?1:-1])
                                                                     ^
test.cpp:5:1: note: in expansion of macro 'MBED_STATIC_ASSERT'
 MBED_STATIC_ASSERT(MBED_LIBRARY_VERSION > 120);
 ^

Assertion failure on ARMC5:

"test.cpp", line 5: Error:  #94: the size of an array must be greater than zero

  MBED_STATIC_ASSERT(MBED_LIBRARY_VERSION > 120);
  ^
test.cpp: 0 warnings, 1 error

Assertion failure on IAR 7.4:

  MBED_STATIC_ASSERT(MBED_LIBRARY_VERSION > 120);
  ^
"C:\Users\chrhas02\mbed\test\test.cpp",5  Error[Pe094]: the size of an array
          must be greater than zero

Errors: 1
Warnings: none

Open to suggestions
cc @theotherjimmy, @pan-

theotherjimmy

💯 👍

jupe · 2016-10-22T16:35:50Z

error messages sounds little strange for me. Could it be more descriptive ?

geky · 2016-10-22T16:53:27Z

I'm open to suggestions if you know of a construct that provides the error more clearly, but unfortunately the error message is a bit inflexible without language support.

The macro can be improved by support for compiler-specific static-assert builtins, but I couldn't find any.

pan- · 2016-10-22T18:05:45Z

@geky It would be nice to add a string describing the failure (similar to static_assert in C++11).
The IAR implementation can rely on the static_assert keyword exposed by the compiler.

jupe · 2016-10-23T06:10:23Z

@kjbracey-arm @SeppoTakalo any thoughts about this ?

geky · 2016-10-23T18:27:15Z

It would be nice to add a string describing the failure

Can do. I was wondering if anyone would request this since I haven't seen the message used for much. An alternative would be to stringize the expr argument as the message.

I also found this construct for gcc in c, but I can't find anything similar for c++03:

#define MBED_STATIC_ASSERT(expr, msg) __extension__ _Static_assert(expr, msg)

kjbracey · 2016-10-24T06:35:15Z

I found static_assert very useful, so put a version in ns_types.h - this does the necessary rooting around to find a real version:

https://github.com/ARMmbed/nanostack-libservice/blob/master/mbed-client-libservice/ns_types.h

But it doesn't fall back to a "portable" one, because I failed to find a construct that worked in all compilers and all circumstances. Including it as a structure member is particularly problematic - and that's what I actually needed first. Would yours work there? Is a function declaration legal in a C structure? Not sure...

Maybe you could borrow the checks before falling back to your base one,.

Figured that one was good enough, because it worked at least on any GCC we were using, and most static asserts would be valid across all compilers, so at least they'd show up on the GCC builds.

Note that the version checking there can be fragile, thanks to compilers amusingly defining the macros from other compilers. ARMCC deciding to define GNUC and the relevant version number without providing the GNU static assert extension was very amusing.

pan- · 2016-10-24T10:40:01Z

@geky

The identifier used is always the same, this prevent usage of multiple static assertions in the same scope. You can cat you identifier with the predefined __LINE__ macro:

#define CONCAT_(lhs, rhs) lhs##rhs
#define CONCAT(lhs, rhs) CONCAT_(lhs, rhs)

// identifier
CONCAT(my_identifier_at_, __LINE__)

I'm not sure if it is a good thing to declare a new symbol which will not be implemented. It might create unwanted or strange conflicts. Maybe using an anonymous enum declaration is better:

#define STATIC_ASSERTION(cond, msg) \
  enum { CONCAT(static_assertion_at_, __LINE__) =  sizeof(char[cond ? 1 : -1]) }

It is would be interesting to have tests which ensure that the static assertion work as expected in every case. It will also help with compiler upgrade.

@kjbracey-arm It is possible to have a portable implementation of static assert which work in C struct; but it will only work outside of a c struct! The trick is to use a zero width bitfield:

The expression that specifies the width of a bit-field shall be an integer constant
expression with a nonnegative value that does not exceed the width of an object of the
type that would be specified were the colon and expression omitted. If the value is zero,
the declaration shall have no declarator.

The bitfield declared does not occupy any size:

A bit-field declaration with no declarator, but only a colon and a width, indicates an
unnamed bit-field.108) As a special case, a bit-field structure member with a width of 0
indicates that no further bit-field is to be packed into the unit in which the previous bit-
field, if any, was placed.

As a result it is possible to write:

#define C_STRUCT_STATIC_ASSERTION(cond, msg) int : cond ? 0 : -1

struct foo_t { 
  int foo;
  /* uncomment this to get a compile time error
  C_STRUCT_STATIC_ASSERTION(0 == 1, "error"); 
  */
  C_STRUCT_STATIC_ASSERTION(1 == 1, "success");
};

kjbracey · 2016-10-24T10:48:36Z

@pan- sneaky, yes, you could have two different macros for block/file versus struct scope. I was trying to come up with a single definition, but that's not a hard requirement.

geky · 2016-10-24T20:38:29Z

It is would be interesting to have tests which ensure that the static assertion work as expected in every case. It will also help with compiler upgrade.

+1, I'll look into adding these

The identifier used is always the same, this prevent usage of multiple static assertions in the same scope.

Is this problematic in the case of a function declaration? Redeclaring the function with the same type should have no issues.

EDIT: This appears to break-down for member functions:
http://stackoverflow.com/a/7518785

I'm not sure if it is a good thing to declare a new symbol which will not be implemented.

It should probably be given an mbed prefix, but with a prefix I'm not sure we should be too concerned with conflicts.

pan- · 2016-10-24T21:39:12Z

Is this problematic in the case of a function declaration? Redeclaring the function with the same type should have no issues.

I'm quoting the spec:

A member shall not be declared twice in the member-specification, except that a nested class or member class template can be declared and then later defined, and except that an enumeration can be introduced with an opaque-enum-declaration and later redeclared with an enum-specifier.

So yes, it is problematic. IIRC, The compilation of the static assert from this PR in a struct fail with ARMCC and IAR.

It should probably be given an mbed prefix, but with a prefix I'm not sure we should be too concerned with conflicts.

Is it not problematic, but it let the possibility to the user to use something unspecified (it will fail at link time,,,); I'm not sure it is the most correct things to do.
In my opinion, if we can avoid holes in our system, it is better for the user, maintainer and supporter.

geky · 2016-10-24T23:10:09Z

@pan- makes good points, so I've updated the pr with the enum construct presented by @pan-. Although I did move the unique-symbol logic into a separate macro in an effort to make the error message cleaner.

Also added tests

I also adopted @kjbracey-arm's compiler checks (and a bit of his documentation ;) ) which gets us the gcc definitions when available, which is a big improvement on gcc builds.

In regards to default behaviour, static assertions need to garuntee program conditions across compilers. It seems risky to just disable the assertions for the majority of C compilers supported by mbed. I went ahead and added the @pan-'s static-struct-assertion to support static assertion's in structs, though I'm not too opinionated on how struct assertions are handled.

Let me know if there are other concerns

pan-

This is a good PR, but it requires few changes to be perfect. There is two points which raise questions:

Should we encourage users (and ourselves) to use MBED_STRUCT_STATIC_ASSERT in C and C++ to enforce consistency ?
Is it possible write test which pass when their compilation fail ? This is the main point of this feature, makes the compilation fail when something incorrect is detected at compile time. Right now, only the case where the assertion should be true are tested. It would be helpful to test the cases where the assertion is false and ensure that the compilation fail as expected.

pan- · 2016-10-25T11:15:08Z

platform/mbed_assert.h

@@ -49,6 +49,65 @@ do {                                                     \
 } while (0)
 #endif

+
+#define _MBED_CONCAT_(a, b) a##b


I'm just wondering if these kind of macro can be packed together. Preprocessor token concatenation is a common operation and in many places across the code base it is not done correctly (arguments not expanded...).

This might be done in another PR.

I'm just wondering if these kind of macro can be packed together.

Good point

What file should these belong to do you think? toolchain.h? new file for preprocessor declarations? mbed_preprocessor.h?

pan- · 2016-10-25T11:28:21Z

platform/mbed_assert.h

+
+#define _MBED_CONCAT_(a, b) a##b
+#define _MBED_CONCAT(a, b) _MBED_CONCAT_(a, b)
+#define _MBED_ASSERTION _MBED_CONCAT(_MBED_ASSERTION_AT_, __LINE__)


_MBED_CONCAT and _MBED_ASSERTION are reserved identifiers according to the C standard:

All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

Whoops, my bad, will fix

pan- · 2016-10-25T11:31:29Z

platform/mbed_assert.h

+ *  }
+ *  @endcode
+ */
+#ifdef __cplusplus


When IAR is used, it is possible to delegate handling of static assertion to the extension static_assert.

pan- · 2016-10-25T11:36:02Z

platform/mbed_assert.h

+/** MBED_STATIC_ASSERT
+ *  Declare compile-time assertions, results in compile-time error if condition is false
+ *
+ *  The assertion acts as a declaration that can be placed at file scope, in a


Is it possible to enhance the documentation by explaining that this macro is valid in C and C++ at file scope and in a code block. Then split the structure case in two:

C++: this macro is valid.

C: should use MBED_STRUCT_STATIC_ASSERT which is also valid for C++ code.

pan- · 2016-10-25T11:41:28Z

platform/mbed_assert.h

+ *  };
+ *  @endcode
+ */
+#if defined(__cplusplus)


This is incorrect, a C++ compiler can treat a structure as a C one if the structure is imported as extern "C".

Testing with gcc 5.3.1, armc 5, and iar 7.4 show this is not the case. That being said I'll go ahead and drop the c++ special case to be safe.

Your correct, "extern C" statement just affect linkage, it doesn't change anything in the compilation process. Indeed, when an element can't be translated to C linkage then it fallback to C++ linkage.

At the end, the change I've requested is not needed.

sg- · 2016-11-01T14:25:06Z

@geky @pan- next steps to move this along??

geky · 2016-11-01T14:44:43Z

Still needs a bit of work for feedback, sounds like another file should be added for preprocessor definitions. mbed_preprocessor.h?

pan- · 2016-11-01T16:58:00Z

@sg- There is two things which can be improved with the PR:

Separate macro concatenation from the static assert module.
Use builtin static_assert in IAR.

Both can be done at a latter stage but is should be referenced at least if it didn't happen in this PR.

sg- · 2016-11-01T18:00:23Z

Lets get them resolved in this PR.

Todos

Separate macro concatenation from the static assert module
Use builtin static_assert in IAR.

Not fully onboard with using a builtin where this seems a language feature, not a tool, but guess it doesn't involve much. #bikesheding

Added MBED_STATIC_ASSERT for compile-time assertions, results in compile-time error if condition is false The assertion acts as a declaration that can be placed at file scope, in a code block (except after a label), or as a member of a C++ class/struct/union. Unfortunately, there does not exist a backup construct for use in C class/struct/union contexts. An alternative macro, MBED_STRUCT_STATIC_ASSERT provides this ability to avoid disabling static assertions for the majority of mbed-supported C compilers.

geky · 2016-11-02T19:23:50Z

@pan-, pr should be updated, let me know if I missed anything

0xc0170 · 2016-11-03T15:01:28Z

Open for review !

pan- · 2016-11-03T15:09:50Z

@geky You forgot to add mbed_preprocesor.h in the last commit.

geky · 2016-11-03T15:27:19Z

Woops! Glad you noticed 😆

sg- · 2016-11-03T23:20:54Z

/morph nightly-build

bridadan · 2016-11-03T23:32:54Z

/morph test-nightly

mbed-bot · 2016-11-04T14:05:59Z

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test-nightly

bridadan · 2016-11-04T14:07:42Z

Failure was my fault, a few machines were stuck in maintenance mode and halted the whole process. Restarting now.

/morph test-nightly

mbed-bot · 2016-11-05T01:01:58Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 999

All builds and test passed!

geky added type: enhancement needs: review labels Oct 21, 2016

theotherjimmy approved these changes Oct 21, 2016

View reviewed changes

geky added needs: work and removed needs: review labels Oct 23, 2016

SeppoTakalo approved these changes Oct 24, 2016

View reviewed changes

geky force-pushed the static-assert branch from 05dae92 to d08fb1a Compare October 24, 2016 22:55

pan- requested changes Oct 25, 2016

View reviewed changes

geky added 2 commits November 1, 2016 21:39

Adopted MBED_STATIC_ASSERT where possible

98db3ab

geky force-pushed the static-assert branch from d08fb1a to 45c4f4b Compare November 2, 2016 19:22

geky added needs: review needs: CI and removed needs: work labels Nov 2, 2016

Added mbed_preprocessor.h to collect common cpp definitions

c11ce21

geky force-pushed the static-assert branch from 45c4f4b to c11ce21 Compare November 3, 2016 15:26

sg- removed needs: review type: enhancement labels Nov 3, 2016

geky added ready for merge and removed needs: CI labels Nov 5, 2016

sg- merged commit 074555b into ARMmbed:master Nov 7, 2016

geky added release-version: 5.2.3 and removed ready for merge labels Nov 7, 2016

Add static assert macro #3113

Add static assert macro #3113

Uh oh!

Conversation

geky commented Oct 21, 2016

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

jupe commented Oct 22, 2016

Uh oh!

geky commented Oct 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- commented Oct 22, 2016

Uh oh!

jupe commented Oct 23, 2016

Uh oh!

geky commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Oct 24, 2016

Uh oh!

pan- commented Oct 24, 2016

Uh oh!

kjbracey commented Oct 24, 2016

Uh oh!

geky commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- commented Oct 24, 2016

Uh oh!

geky commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sg- commented Nov 1, 2016

Uh oh!

geky commented Nov 1, 2016

Uh oh!

pan- commented Nov 1, 2016

Uh oh!

sg- commented Nov 1, 2016 • edited by geky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todos

Uh oh!

geky commented Nov 2, 2016

Uh oh!

0xc0170 commented Nov 3, 2016

Uh oh!

pan- commented Nov 3, 2016

Uh oh!

geky commented Nov 3, 2016

Uh oh!

sg- commented Nov 3, 2016

Uh oh!

bridadan commented Nov 3, 2016

Uh oh!

geky commented Oct 22, 2016 •

edited

Loading

geky commented Oct 23, 2016 •

edited

Loading

geky commented Oct 24, 2016 •

edited

Loading

geky commented Oct 24, 2016 •

edited

Loading

sg- commented Nov 1, 2016 •

edited by geky

Loading