-
Notifications
You must be signed in to change notification settings - Fork 407
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
Don't allow calling UnorderdMap::value_at for a set #4639
Don't allow calling UnorderdMap::value_at for a set #4639
Conversation
3ff24b4
to
e4be7a0
Compare
@@ -699,7 +725,8 @@ class UnorderedMap { | |||
/// kernel. | |||
KOKKOS_FORCEINLINE_FUNCTION | |||
key_type key_at(size_type i) const { | |||
return m_keys[i < capacity() ? i : capacity()]; | |||
KOKKOS_EXPECTS(i < capacity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The precondition check needs to be guarded for deprecated code enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that returning the last element if out-of-range was just wrong/buggy. That was more like a drive-by change. If that is debatable I'm happy to just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kokkos/containers/src/Kokkos_UnorderedMap.hpp
Line 313 in 01974b7
m_keys("UnorderedMap keys", capacity() + 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not defending the current design, just pointing out that the behavior is changing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See b058e08.
value_at(size_type i) const { | ||
return m_values[is_set ? 0 : (i < capacity() ? i : capacity())]; | ||
KOKKOS_EXPECTS(i < capacity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about change of behavior
b058e08
to
586e7af
Compare
@Char-Aznable would you please review? |
Looks good to me! Thanks for the patch! |
Fixes #4555.