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

Bytes type and coverall test fixture #16

Merged
merged 4 commits into from
Jul 11, 2024
Merged

Conversation

skeet70
Copy link
Member

@skeet70 skeet70 commented Jul 3, 2024

Switched from Disposable to AutoClosable. Big difference is that AutoClosable throws Exception which makes it slightly more annoying in some use cases. It allows us to use try-with-resources though to make sure things that need to get destroyed do get destroyed. We can't inline functions like Kotlin is doing with destroy for Disposable, so we have to hold a strong reference to be able to destroy something after a block of code, which negates a bit of the point.

Disposable could be added back in the future (or in addition to AutoClosable) if necessary, but looking through the Rust code I didn't see anywhere a destroy was being presented from that side or anything.

Writing the coverall tests did catch a few edge cases that were fixed in this PR.

@skeet70 skeet70 requested a review from a team as a code owner July 3, 2024 16:25
@skeet70 skeet70 requested review from BobWall23, coltfred and giarc3 and removed request for a team and BobWall23 July 3, 2024 16:25

@Override
public long allocationSize(byte[] value) {
return 4L + (long)value.length;
Copy link
Member

Choose a reason for hiding this comment

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

length always 32 bits, even on 64 bit architectures? Looks like Java doesn't have sizeof(), so doing something different would probably suck anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

The return value from Java for length is always an int regardless of arch/JVM. The actual max size of the array is JVM/arch dependant, but pretty sure it's always within int max.

this((Pointer){%- call java::to_ffi_call(cons) -%});
}
{%- endif %}
{%- when None %}
{%- endmatch %}

@Override
public void destroy() {
public synchronized void close() {
Copy link
Member

Choose a reason for hiding this comment

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

Since this is close() instead of destroy(), should it be wasClosed instead of wasDestroyed? Maybe wasDestroyed is fine, since it looks like it does destroy the fields when closing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's a bit of both. close is the Java parlance and the Java interface we've implemented, but destroy is the common-across-bindgens uniffi word for it.

public RustBuffer.ByValue getValue() {
Pointer pointer = this.getPointer();
RustBuffer.ByValue value = new RustBuffer.ByValue();
value.writeField("capacity", pointer.getLong(0));
Copy link
Member

Choose a reason for hiding this comment

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

And the Rust ones are always 64 bits for capacity and length, regardless of architecture?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a construct just for usage with uniffi, so yeah the capacity and length is always 64 bit.

src/templates/Helpers.java Outdated Show resolved Hide resolved
@skeet70 skeet70 merged commit cd4eb0e into 6-futures Jul 11, 2024
4 of 6 checks passed
@skeet70 skeet70 deleted the 14-bytes-and-coverall branch July 11, 2024 20:56
skeet70 added a commit that referenced this pull request Jul 11, 2024
* First pass at async file, still need futures

* Futures test passed!

I don't believe it... need some more complex tests

* Update src/templates/UniffiHandleMap.java

Co-authored-by: Bob Wall <[email protected]>

* Update src/templates/UniffiHandleMap.java

Co-authored-by: Bob Wall <[email protected]>

* Checkpointing to update another branch

* Going to require implementing callbacks anyway

* So close to working async_traits

Right now `CallbackTemplate.java` isn't being included in the
generation, causing all build failures

* One compilation error away...

from probably the next batch of compilation errors!

* Compiles, runtime error and need to fill out tests

* Structures don't allow boxed type class fields

need to make everything primitives if possible. If not, need to make
primitives around structures.

* Appropriate documentation is important.

Structures require a no-args constructor in almost all cases. The Kotlin
one happened to be working because all args were defaultable, which from
the Java view works as a no-args constructor. Nothing about this is
mentioned in the Structure or JNA documentation.

* Sooooooo close.

Something going on with cancellation looks like it's the cause of the
only remaining failures (commented out).

* Only cancellation failing

Async callbacks weren't waiting on `get` correctly. Once that was fixed
needed to unwrap `ExecutionException`s so they could be passed to Rust
callbacks correctly.

* Normal future cancellation fixed.

* Fixed trait future cancellation too

* Clean up some TODOs after PR comment pass

* What if CI just needs more time?

* Add a little more info to assert fail

* Only run the failing test

* Touch to rebuild

* Add all tests back in

* Bytes type and coverall test fixture (#16)

* Passing tests, coverall generation working

Still need to complete coverall tests

* Checkpointing because I don't want to lose it

* Finished all coverall tests

* rogue println

---------

Co-authored-by: Bob Wall <[email protected]>
Co-authored-by: Colt Frederickson <[email protected]>
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