added xrange adaptor and fix for xstepped_range size #139
Conversation
|
I've added tests to this PR. Note the fix for xstepped_range. Because of default floor rounding, the last value was not selected in some cases. E.g. |
include/xtensor/xslice.hpp
Outdated
| { | ||
| } | ||
|
|
||
| template <class MIN = A, class MAX = B, class STEP = C> |
There was a problem hiding this comment.
MIN and MAX should be avoided as names, they can lead to conflict with potentially MIN and MAX macros (I know it is really ugly but you can't imagine the number of such horrors you can find in production code). It could be MINI and MAXI or whatever else, as long as it can't conflict with macros.
include/xtensor/xslice.hpp
Outdated
| template <class A, class B, class C> | ||
| struct xrange_adaptor | ||
| { | ||
| xrange_adaptor(A min, B max, C step) |
There was a problem hiding this comment.
min and max should be avoided as names, like their uppercase counterpart. Windows define min and max macros lowercase (yes, I'm serious ...); even if we can prevent their generation with the NOMINMAX macro, we should avoid possible conflicts.
include/xtensor/xslice.hpp
Outdated
| else | ||
| { | ||
| return xstepped_range<int>(size - 1, m_max, m_step); | ||
| } |
There was a problem hiding this comment.
I think you can simplify a bit this code:
std::size_t min_arg = m_step > 0 ? 0 : size - 1;
return xstepped_range<int>(min_arg, m_max, m_step);
include/xtensor/xslice.hpp
Outdated
| else | ||
| { | ||
| return xstepped_range<int>(m_min, -1, m_step); | ||
| } |
include/xtensor/xslice.hpp
Outdated
| else | ||
| { | ||
| return xstepped_range<int>(size - 1, -1, m_step); | ||
| } |
include/xtensor/xslice.hpp
Outdated
| }; | ||
|
|
||
| template <class A, class B> | ||
| inline auto range(A min, B max) |
There was a problem hiding this comment.
Same remark as previous about min and max names.
There was a problem hiding this comment.
should the "old" range functions also change the parameter names?
There was a problem hiding this comment.
Yes, that something we missed when the first implementation was merged.
include/xtensor/xslice.hpp
Outdated
| } | ||
|
|
||
| template <class A, class B, class C> | ||
| inline auto range(A min, B max, C step) |
include/xtensor/xslice.hpp
Outdated
|
|
||
| template <class MIN = A, class MAX = B, class STEP = C> | ||
| std::enable_if_t<!std::is_integral<MIN>::value && std::is_integral<MAX>::value && std::is_integral<STEP>::value, xstepped_range<int>> | ||
| get(std::size_t size) |
There was a problem hiding this comment.
You can inline this function and its overloads.
include/xtensor/xslice.hpp
Outdated
| inline auto xnone() | ||
| { | ||
| return xnone_tag(); | ||
| } |
There was a problem hiding this comment.
I think we can use placeholders here, so we have a really cool syntax:
auto v = view(exp, range(2, _, 2));Although _ is not a in the STL yet, it is in the MPL and it may become available in the STL in the future. So we have to be very cautious. A good approach would be to nest it in a placeholder namespace, forcing the client to explicitly use the namespace if he wants to use the placeholder syntax. This way, when the _ placeholder comes in the standard, we can just replace our code by an import of the standard one.
A possible implementation:
// The following is nested in xt namespace of course
namespace placeholders
{
// xtensor universal placeholder
struct xtuph
{
};
constexpr xtuph _{};
}
|
Closes #120. Good job ! |
|
cool! |
still missing tests.