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.

Sign up for GitHub

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

Jump to bottom

making adapt(std::array) return xfixed_adaptor. #2350

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

making adapt(std::array) return xfixed_adaptor. #2350

wants to merge 2 commits into from

Conversation

vakokako
Copy link

@vakokako vakokako commented Apr 6, 2021

This implements #2337.

Making adapt(std::array) return xfixed_adaptor instead of xtensor_adaptor.

@vakokako vakokako force-pushed the make_adapt_return_xfixed_adaptor branch from 029a894 to 299be1d Compare April 6, 2021 16:19
JohanMabille
JohanMabille reviewed Apr 7, 2021
View reviewed changes
Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! I think it is acceptable to provide this implementation on Linux / OSX only, and keep the previous one on Windows. We have a long story of ICEs with MSVC compilers, and we dropped support for fixed container on Windows.

include/xtensor/xadapt.hpp
@@ -209,7 +209,8 @@ namespace xt
* @param container the container to adapt
* @param l the layout_type of the xtensor_adaptor
*/
template <layout_type L = XTENSOR_DEFAULT_LAYOUT, class C>
template <layout_type L = XTENSOR_DEFAULT_LAYOUT, class C,
XTL_REQUIRES(detail::not_an_array<std::decay_t<C>>)>
inline xtensor_adaptor<C, 1, L>
Copy link
Member

Choose a reason for hiding this comment

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

I think the return type should be xtensor_adaptor<xtl::closure_type_t<C>, 1, L>, according to the implementation below.

Copy link
Author

Choose a reason for hiding this comment

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

yep, but I didn't really want to change existing implementation, as I'm not too deep into it)

include/xtensor/xfixed.hpp
@@ -400,7 +400,7 @@ namespace xt
struct xcontainer_inner_types<xfixed_adaptor<EC, S, L, SH, Tag>>
{
using storage_type = std::remove_reference_t<EC>;
using reference = typename storage_type::reference;
using reference = inner_reference_t<storage_type>;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@vakokako
Copy link
Author

vakokako commented Apr 7, 2021

Thanks for implementing this! I think it is acceptable to provide this implementation on Linux / OSX only, and keep the previous one on Windows. We have a long story of ICEs with MSVC compilers, and we dropped support for fixed container on Windows.

interesting, what makes the msvc fail?

@JohanMabille
Copy link
Member

interesting, what makes the msvc fail?

That's an Internal Compiler Error, so it is hard to say. But I would bet on an out of memory error due to too many template instantiations.

@vakokako vakokako force-pushed the make_adapt_return_xfixed_adaptor branch from 68c803b to 4929a96 Compare April 8, 2021 16:41
@vakokako vakokako force-pushed the make_adapt_return_xfixed_adaptor branch from 4929a96 to dc1378f Compare April 8, 2021 16:42
@emmenlau
Copy link
Contributor

emmenlau commented Apr 9, 2021

Thanks for implementing this! I think it is acceptable to provide this implementation on Linux / OSX only, and keep the previous one on Windows. We have a long story of ICEs with MSVC compilers, and we dropped support for fixed container on Windows.

Hi Johan! Just to clarify, do you mean this PR should be changed to provide the previous implementation on Windows, or can it be merged as is? Thanks for your help!

@JohanMabille
Copy link
Member

Hi Mario! I think it should be changed to keep the previous implementation on Windows to avoid ICE and weird failures due to compiler bugs.

@emmenlau
Copy link
Contributor

Hi Mario! I think it should be changed to keep the previous implementation on Windows to avoid ICE and weird failures due to compiler bugs.

Good, I'll ping Mykola about this and we'll see to update the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohanMabille JohanMabille JohanMabille left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vakokako @JohanMabille @emmenlau