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.

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

xclosure #85

Merged
merged 1 commit into from
Feb 1, 2017
Merged

xclosure #85

merged 1 commit into from
Feb 1, 2017

Conversation

SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Jan 27, 2017

For expressions:

  • The closure type for an lvalue reference is a const reference.
  • The closure type for anything else is a value.

For scalars:

  • The closure type is an xscalar

This should solve #75.

@@ -71,9 +66,10 @@ namespace xt

template <class F, class R>
template <class... E>
inline auto xvectorizer<F, R>::operator()(const E&... e) const -> get_xfunction_type<E...>
inline auto xvectorizer<F, R>::operator()(E&&... e) const
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: re-add -> get_xfunction_type?

@SylvainCorlay
Copy link
Member Author

Heads up to @wolfv since this is going to change how one writes ufuncs.

@wolfv
Copy link
Member

wolfv commented Feb 1, 2017

Thanks!
Out of curiosity, would you mind to elaborate a tiny bit on what the xclosure is and why it's needed?

@SylvainCorlay
Copy link
Member Author

The current mechanism may result in dangling references.

Closure types are how expressions hold their different components (xfunction, xbroadcast). In x + y, the members of the expression (x and y) are stored by the xfunction in a tuple of "closure types".

In the old mechanism, when using an xfunction on an rvalue xarray (e.g built by a function), the closure type was always a const reference, the xfunction is then holding a const reference to a temporary, resulting in a dangling reference when composing it in another expression.

The new mechanism is different. An expression built with a rvalue will hold a value, and move the rvalue to it. An expression built with an lvalue reference will hold a const reference to it.

@wolfv
Copy link
Member

wolfv commented Feb 1, 2017

Thanks, good to know! I can also confirm that this PR is fixing the view iterator error I was constantly seeing 🎉

@SylvainCorlay
Copy link
Member Author

Cool, that is good to hear.

@SylvainCorlay
Copy link
Member Author

@JohanMabille yay!

@JohanMabille
Copy link
Member

Let's rock !

@JohanMabille JohanMabille merged commit 47337c2 into xtensor-stack:master Feb 1, 2017
@SylvainCorlay SylvainCorlay deleted the closure branch February 1, 2017 23:07
@SylvainCorlay SylvainCorlay mentioned this pull request Feb 2, 2017
@SylvainCorlay
Copy link
Member Author

Ping @juanalday. You might be interested in this fix. #101 also somplifies this.

@SylvainCorlay
Copy link
Member Author

@JohanMabille this also fixed the issue when replacing xscalar by 0-D xtensors or xarrays in the code base. So it is a good sanity check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants