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

[proposed feature] Allow AutoElements to store an owned local reference #508

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jrose-signal
Copy link
Contributor

Overview

Currently, AutoElements borrows a local reference to a JPrimitiveArray. However, this makes it harder to pass the AutoElements around and make collections of them, since they depend not only on the context but on the local variable that held the array they were borrowed from. This PR changes AutoElements to depend on anything that can AsRef to a JPrimitiveArray, which in practice means either a reference or an owned local reference, or even an AutoLocal.

Another option would be for AutoElements to always create a new local reference, but that could break programs that aren't expecting to allocate more local references just by using AutoElements.

This is a breaking change, since it changes the generic signature of AutoElements.

Is this useful enough to take?

Definition of Done

  • There are no TODOs left in the code
  • Change is covered by automated tests: I manually messed with the existing AutoElements test to make sure this worked, but I don't want to add a ton of duplication to the tests.
  • The coding guidelines are followed
  • Public API has documentation: Haven't updated this yet.
  • User-visible changes are mentioned in the Changelog: Or this.
  • The continuous integration build passes

(rather than requiring a borrow)

This is a breaking change, since it changes the generic signature of
AutoElements.
@jrose-signal
Copy link
Contributor Author

This is not the same thing as #302 but it might be enough of a usability improvement anyway.

@jrose-signal
Copy link
Contributor Author

Ran into a need for this again today, can rebase if there's interest.

@argv-minus-one
Copy link
Contributor

Seems reasonable to me. @rib, how do you feel about this?

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.

2 participants