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
add caching for view strides #652
Conversation
include/xtensor/xview.hpp
Outdated
@@ -166,7 +166,7 @@ namespace xt | |||
data() const; | |||
|
|||
template <class T = xexpression_type> | |||
std::enable_if_t<has_raw_data_interface<T>::value, strides_type> | |||
std::enable_if_t<has_raw_data_interface<T>::value, strides_type&> |
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 return type should be const strides_type&
include/xtensor/xview.hpp
Outdated
if (m_strides_computed) | ||
{ | ||
return m_strides; | ||
} |
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 prefer something like:
if(!m_strides_computed)
{
m_strides = compute_strides();
m_strides_computed = true;
}
return m_strides;
for the following reasons:
- you avoid mutiple return points
- all the caching logic is done in the same place, and
compute_strides
is now a stateless function (that's more flexible for future refactoring / cache policy changes etc).
include/xtensor/xview.hpp
Outdated
{ | ||
return m_strides; | ||
} | ||
m_strides = xtl::make_sequence<strides_type>(dimension(), 0); |
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.
This shows another bug in the computaion of the strides: the new_axis
were not taken into account in the previous code. Now that you fixed that, you also need to fix the loop.
Also the failing case that triggered the bug should be added to the test suite |
a0e7b00
to
341baba
Compare
This fixes #651