Implement apply with std 17 #2835
Conversation
spectre-ns
force-pushed
the
apply17
branch
2 times, most recently
from
6ed414e to
569b615
Compare
include/xtensor/utils/xutils.hpp
Outdated
| { | ||
| return self(self, std::size_t{i + 1}, t...); | ||
| } | ||
| XTENSOR_THROW(std::runtime_error, "Index applied to tuple of insufficient length"); |
There was a problem hiding this comment.
@JohanMabille I think the noexcept specification here was too restrictive. We need to provide some way of indicating when index exceeds sizeof...(S). In the old implementation I think we would simply get an access violation which is not ideal.
There was a problem hiding this comment.
The initial idea was to disable all checks to get the most opitmized generated code; I don't remember whether this function is used in critical parts of the code, so I cannot say for the NOEXCEPT, but I think we should prefer XTENSOR_ASSERT to XTENSOR_THROW.
There was a problem hiding this comment.
Moved to assert. Clang compiles this code nicely... MSVC was a bit of a mess but I'm not sure if it's a code generation issue or just noise.
There was a problem hiding this comment.
This is a link to the generated code. https://godbolt.org/z/sMdT517G4
It can calulate the result at compile time. Can I say it is the optimal solution then?
| { | ||
| using FT = std::add_pointer_t<R(F&&, const std::tuple<S...>&)>; | ||
| static const std::array<FT, sizeof...(I)> ar = {{&apply_one<R, F, I, S...>...}}; | ||
| return ar[index](std::forward<F>(func), s); |
There was a problem hiding this comment.
Potential access violation if index > I - 1.
include/xtensor/utils/xutils.hpp
Outdated
| return std::apply( | ||
| [&](const S&... args) -> R | ||
| { | ||
| return [&](const auto&... t) -> R |
There was a problem hiding this comment.
Why do we need an additional lambda here? Can't we have the implementation directly in the lambda passed to std::apply ?
There was a problem hiding this comment.
Oops. Yeah... My bad.
spectre-ns
force-pushed
the
apply17
branch
3 times, most recently
from
12be501 to
cbf4fc0
Compare
|
@JohanMabille I restored the noexcept specification and used an ASSERT as requested. Should be good to go! |
|
Moved the noexcept assert to static assertions as they are known at compile time. |
spectre-ns
force-pushed
the
apply17
branch
2 times, most recently
from
9befbea to
464a7c5
Compare
|
Thanks a lot! |
2 participants
Checklist