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

Address issue #229 and fix missing noexcept #230

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented Jan 16, 2023

The conversion operators from layout_stride for both layout_left and layout_right were missing a static_cast and also were not marked noexcept. I now guard the precondition check with NDEBUG.

Note: the throw in there is legal: it will result in a termination if triggered since the function is marked noexcept.

@crtrott
Copy link
Member Author

crtrott commented Jan 16, 2023

This addresses issue #229 hopefully.

throw std::runtime_error("Assigning layout_stride to layout_left with invalid strides.");
// Note this throw will lead to a terminate if triggered since this function is marked noexcept
Copy link
Member

Choose a reason for hiding this comment

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

why not just call assert in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify @dalg24 's comment: assert does something exactly when not defined(NDEBUG) is true, so this should behave effectively like assert. Furthermore, an actual assert may have a better error message, as a noexcept violation might not need to print the exception message.

The nicer way to do this would be to define an MDSPAN_ASSERT macro that Does The Right Thing, but I think it's fine for now just to use assert.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would need to write a function I call inside the assert I guess? Because I don't want to execute the loop unless !NDEBUG

Or do you just want me to replace the innermost if/throw?

Copy link
Member

Choose a reason for hiding this comment

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

immediately invoked lambda code was worse. reverted to the initial PR

Copy link
Contributor

@mhoemmen mhoemmen left a comment

Choose a reason for hiding this comment

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

I'm OK with merging this now, but I think @dalg24 has a good point that assert would better express intent. It might also have a better error reporting experience.

The conversion operators from layout_stride for both layout_left
and layout_right were missing a static_cast and also were not marked
noexcept. I now guard the precondition check with NDEBUG.

Note: the throw in there is legal: it will result in a termination
if triggered since the function is marked noexcept.
@crtrott crtrott merged commit 8b437ba into kokkos:stable Jan 24, 2023
@crtrott crtrott deleted the fix_noexcept_missing branch January 24, 2023 21:56
mhoemmen added a commit to mhoemmen/mdspan that referenced this pull request Jul 26, 2023
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.

3 participants