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.

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: force Zephyr BIT/MIN/MAX/CLAMP definitions #56629

Closed
wants to merge 3 commits into from

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Apr 6, 2023

We've been guarding the definition of BIT/MIN/MAX/CLAMP macros as they are commonly found in other projects. This is dangerous, because depending on the order of includes, Zephyr code could end up using macros defined by other projects (e.g. via HALs).

The current #ifndef protection goes against the recently established guidelines in #51963, so let's remove it.

Ref. https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-3-macro-name-collisions

@zephyrbot zephyrbot added the area: Base OS Base OS Library (lib/os) label Apr 6, 2023
@zephyrbot zephyrbot requested review from andyross and dcpleung April 6, 2023 16:24
@gmarull gmarull changed the title sys: util: force Zephyr MIN/MAX/CLAMP definitions sys: util: force Zephyr BIT/MIN/MAX/CLAMP definitions Apr 6, 2023
@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Apr 11, 2023
@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 11, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_espressif zephyrproject-rtos/hal_espressif@d7576d0 zephyrproject-rtos/hal_espressif#208 zephyrproject-rtos/hal_espressif#208/files
hal_nxp zephyrproject-rtos/hal_nxp@d3c964c zephyrproject-rtos/hal_nxp#230 zephyrproject-rtos/hal_nxp#230/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@sylvioalves
Copy link
Collaborator

@gmarull, hal_espressif PR merged. I'll approve when this PR manifest gets update as well.

@gmarull gmarull force-pushed the no-ifndef-util branch 2 times, most recently from bf73a0f to 90605bf Compare April 12, 2023 15:07
marekmatej
marekmatej previously approved these changes Apr 12, 2023
Copy link

@marekmatej marekmatej left a comment

Choose a reason for hiding this comment

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

LGTM

gmarull added 3 commits April 14, 2023 10:47
- Sort includes
- Remove some Kernel internal includes
- Make sure Zephyr headers come first, so Zephyr's BIT() doesn't
  conflict with HAL.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
Avoid redefinition errors due to HAL defining BIT() macro in public
headers.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
We've been guarding the definition of BIT/MIN/MAX/CLAMP macros as they
are commonly found in other projects. This is dangerous, because
depending on the order of includes, Zephyr code could end up using
macros defined by other projects (e.g. via HALs).

The current #ifndef _protection_ goes against the recently established
guidelines in zephyrproject-rtos#51963, so let's remove it.

Ref.
https://docs.zephyrproject.org/latest/contribute/coding_guidelines/
index.html#rule-a-3-macro-name-collisions

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jun 14, 2023
@github-actions github-actions bot closed this Jun 28, 2023
@gmarull gmarull deleted the no-ifndef-util branch June 5, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Base OS Base OS Library (lib/os) DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_espressif manifest-hal_nxp Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants