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

libmetal(cmake): set HAVE_STDATOMIC_H default true in NuttX platform #316

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

wyr-7
Copy link
Contributor

@wyr-7 wyr-7 commented Oct 10, 2024

there is no need to use cmake include detection under NuttX platform


# there is no need to use cmake include detection
# under NuttX platform
set(HAVE_STDATOMIC_H true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems not a good idea to force HAVE_STDATOMIC_H here
what about ujsrt having

elseif (NOT "${CMAKE_SYSTEM_NAME}" STREQUAL "NuttX")

  # TODO: fix for find_path() to detect stdatomic.h
  # find_path (HAVE_STDATOMIC_H stdatomic.h)
  set (_saved_cmake_required_flags ${CMAKE_REQUIRED_FLAGS})
  set (CMAKE_REQUIRED_FLAGS "-c" CACHE STRING "")
  check_include_files (stdatomic.h HAVE_STDATOMIC_H)
  set (CMAKE_REQUIRED_FLAGS ${_saved_cmake_required_flags})

endif ()

Copy link
Contributor

Choose a reason for hiding this comment

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

NuttX platform supports builtin atomic and toolchain atomic.
The reason why CMAKE_REQUIRED_FLAGS cannot be used here is because this cache will destroy the CMake build of the integrated platform

Copy link
Contributor

Choose a reason for hiding this comment

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

My point here is that HAVE_STDATOMIC_H is probably defined in NuttX so the set(HAVE_STDATOMIC_H true) seems redundant.
Exclude the cmake code related to CMAKE_REQUIRED_FLAGS could be sufficient

Copy link
Contributor

Choose a reason for hiding this comment

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

My point here is that HAVE_STDATOMIC_H is probably defined in NuttX so the set(HAVE_STDATOMIC_H true) seems redundant. Exclude the cmake code related to CMAKE_REQUIRED_FLAGS could be sufficient

HAVE_STDATOMIC_H is the result of following this check check_include_files (stdatomic.h HAVE_STDATOMIC_H).

sure, thanks for your suggestion @arnopo .
Let's check for non-NuttX platforms in the if block

cmake/depends.cmake Outdated Show resolved Hide resolved
there is no need to use cmake include detection under NuttX platform

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@wyr-7
Copy link
Contributor Author

wyr-7 commented Oct 18, 2024

@arnopo Thansk, I have updated it according to @xuxin930 comment.

Copy link
Contributor

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Looks good to go.

@arnopo arnopo merged commit 4d0812d into OpenAMP:main Oct 21, 2024
2 checks passed
@arnopo
Copy link
Contributor

arnopo commented Oct 21, 2024

I have fixed the commit subject that was not up to date

@arnopo arnopo added this to the Release v2024.10 milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants