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

Apply check functions to common functions #129

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

@yasahi-hpc yasahi-hpc commented Aug 29, 2024

Improves #80

This PR aims at replacing runtime assert with KOKKOSFFT_EXPECTS. Adding more static_assertions if applicable.
Modifications are applied to source code under common/src and common/unit_test.

Following modifications are made

  • Apply are_valid_axes function where it is applicable
  • Real to real case is suppressed by static_assertion in KokkosFFT_layout.hpp
  • Apply KOKKOSFFT_EXPECTS to some if else conditional (throw std::runtime_error in the case of else)
  • Replacing is_view<ViewType>::value with is_view_v<ViewType>

@yasahi-hpc yasahi-hpc self-assigned this Aug 29, 2024
@tpadioleau tpadioleau self-requested a review September 6, 2024 07:31
Copy link
Member

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

Seems globally good to me!

Not directly related to his PR but I see you rely on the standard version to use std::source_location. I think it is more reliable to use feature testing like #if defined(__cpp_lib_source_location). This way you would be compatible with a compiler that supports only a part of C++ 20.

common/src/KokkosFFT_normalization.hpp Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

Seems globally good to me!

Thank you for your reviews!

Not directly related to his PR but I see you rely on the standard version to use std::source_location. I think it is more reliable to use feature testing like #if defined(__cpp_lib_source_location). This way you would be compatible with a compiler that supports only a part of C++ 20.

I have fixed accordingly. My aims was to make sure that C++ 20 build actually relies on std::source_location rather than using C++17 version in case compiler does not support it.

Copy link
Member

@tpadioleau tpadioleau left a comment

Choose a reason for hiding this comment

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

Good to me!

@yasahi-hpc yasahi-hpc merged commit 0dfbae8 into kokkos:main Sep 6, 2024
20 checks passed
@yasahi-hpc yasahi-hpc deleted the improve-assertions branch September 6, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants