Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys: util: Prevent multiple definitions of ARRAY_SIZE #50239

Closed
wants to merge 4 commits into from
Closed

sys: util: Prevent multiple definitions of ARRAY_SIZE #50239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c96bc2
sys: util: Prevent multiple definitions of ARRAY_SIZE
mfischer Sep 14, 2022
2922060
tests: unit: Add explicit ARRAY_SIZE tests
mfischer Sep 14, 2022
9542e34
sys: util: Take stronger stance on MIN()/MAX()
mfischer Oct 5, 2022
37cf75d
doc/release-note-3.3: Add notes about MAX/MIN/ARRAY_SIZE
mfischer Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/releases/release-notes-3.3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ Deprecated in this release

Stable API changes in this release
==================================
* System Utility Headers

* Prefixed :c:func:`MIN` and :c:func:`MAX` and :c:func:`ARRAY_SIZE` with `#undef` to prevent
accidential multiple definitions when dealing with third party libraries that bring their
own definitions.
This is a departure from previous behavior where include order of header files mattered.

New APIs in this release
========================
Expand Down
8 changes: 4 additions & 4 deletions include/zephyr/sys/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ extern "C" {
/* The built-in function used below for type checking in C is not
* supported by GNU C++.
*/
#undef ARRAY_SIZE
#define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0]))

#else /* __cplusplus */
Expand All @@ -105,6 +106,7 @@ extern "C" {
*
* In C, passing a pointer as @p array causes a compile error.
*/
#undef ARRAY_SIZE
#define ARRAY_SIZE(array) \
((size_t) (IS_ARRAY(array) + (sizeof(array) / sizeof((array)[0]))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice this means ARRAY_SIZE implementation will depend on the including order: BAD. Note that since Zephyr seems to be worried about safety, this violates MISRA 20.5 (advisory) as well.

I think we should either:

  • Fix offending libraries/3rd party code
  • Use a namespace in Zephyr, to protect us from 3rd party code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice this means ARRAY_SIZE implementation will depend on the including order: BAD. Note that since Zephyr seems to be worried about safety, this violates MISRA 20.5 (advisory) as well.

Valid arguments.

I think we should either:

* Fix offending libraries/3rd party code

Yes.

* Use a namespace in Zephyr, to protect us from 3rd party code.

There is always chance for collision, and system should not move out of the way.
Choosing SYS_ or Z_ prefix will fix a problem until other system gets the same idea and some interface header for some library, hal or other thing does the #ifndef/#define thing in interface header again.

Only way to fix header order problem is to avoid having definitions, in headers, that should not be there.

sys/util.h is included specifically for the definitions it provides, so it is not guilty of redefinitions.

Question: how many Zephyr headers include sys/util.h for no reason?


Expand Down Expand Up @@ -237,7 +239,6 @@ extern "C" {
#define ceiling_fraction(numerator, divider) \
(((numerator) + ((divider) - 1)) / (divider))

#ifndef MAX
/**
* @brief Obtain the maximum of two values.
*
Expand All @@ -249,10 +250,9 @@ extern "C" {
*
* @returns Maximum value of @p a and @p b.
*/
#undef MAX
#define MAX(a, b) (((a) > (b)) ? (a) : (b))
#endif

#ifndef MIN
/**
* @brief Obtain the minimum of two values.
*
Expand All @@ -264,8 +264,8 @@ extern "C" {
*
* @returns Minimum value of @p a and @p b.
*/
#undef MIN
#define MIN(a, b) (((a) < (b)) ? (a) : (b))
#endif

#ifndef CLAMP
/**
Expand Down
10 changes: 10 additions & 0 deletions tests/unit/util/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ ZTEST(util_cxx, test_ARRAY_INDEX)
run_ARRAY_INDEX();
}

ZTEST(util_cxx, test_ARRAY_SIZE)
{
run_ARRAY_SIZE();
}

ZTEST(util_cxx, test_PART_OF_ARRAY)
{
run_PART_OF_ARRAY();
Expand Down Expand Up @@ -234,6 +239,11 @@ ZTEST(util_cc, test_ARRAY_INDEX)
run_ARRAY_INDEX();
}

ZTEST(util_cc, test_ARRAY_SIZE)
{
run_ARRAY_SIZE();
}

ZTEST(util_cc, test_PART_OF_ARRAY)
{
run_PART_OF_ARRAY();
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/util/test.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,3 +526,9 @@ void run_ARRAY_INDEX_FLOOR(void)

zassert_equal(array[ARRAY_INDEX_FLOOR(array, &alias[1])], 0);
}

void run_ARRAY_SIZE(void)
{
size_t array[] = {1, 2, 3, 4};
zassert_equal(ARRAY_SIZE(array), 4);
}