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

Fix #641 #644

Merged
merged 15 commits into from
Aug 21, 2024
Merged

Fix #641 #644

merged 15 commits into from
Aug 21, 2024

Conversation

ambiguousname
Copy link
Member

Fixes #641

@ambiguousname ambiguousname added the B-js JS backend label Aug 17, 2024
tool/src/js/type_generation/converter.rs Outdated Show resolved Hide resolved
&param.ty,
param_info.name.clone(),
struct_borrow_info.as_ref(),
alloc,
));
None,
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I still don't think this is the best way to structure this

I don't think this parameter should be necessary. We're manually doing slice.ptr, slice.size above, so what we have is that two codepaths calling gen_js_to_c_for_type, one which uses ...{}.splat() to splat it, and one which manually spreads it, and we have this conversion parameter to deal with that divergence.

gen_js_to_c_for_type already unconditionally returns a ...struct.toFFI() for structs, so all of its call sites already expect spread operators. I think it should unconditionally return ...slice.splat() for slices (with caveats on the arena behavior). The call sites should be adjusted to handle that for slices; that will simplify them since currently this call site special-cases slices and manually spreads it.

Copy link
Contributor

Choose a reason for hiding this comment

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

(all of this is an unfortunate consequence of how slices need to be special snowflakes for borrowing, which makes it easy to accidentally write code tha treats them as more special than they need to be)

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 probably for the best that we try to aim for near-universal usage of ...splat() in gen_js_to_c, it's just an absolute pain for freeing. I think I'll be able to make a clean solution though.

@ambiguousname
Copy link
Member Author

ambiguousname commented Aug 21, 2024

@Manishearth ... ___ .splat() is now always returned by gen_js_to_c. The lifetimeEdges code looks a little weird, but I think it still works.

Here's some sample code for inside of methods, for reference, on Foo:

static new_(x) {
        let functionGarbageCollector = new diplomatRuntime.GarbageCollector();

        const xSlice = [...functionGarbageCollector.alloc(diplomatRuntime.DiplomatBuf.str8(wasm, x)).splat()];
        
        // This lifetime edge depends on lifetimes 'a
        let aEdges = [xSlice];
        const result = wasm.Foo_new(...xSlice);
    
        try {
            return new Foo(diplomatRuntime.internalConstructor, result, [], aEdges);
        }
        
        finally {
            functionGarbageCollector.garbageCollect();}
}

@Manishearth Manishearth merged commit c43f257 into rust-diplomat:main Aug 21, 2024
16 checks passed
@Manishearth
Copy link
Contributor

I think a potentially cleaner way to do the allocation would be to pass in an allocation retainer object into the ctor for these slice objects, but this works too.

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

Successfully merging this pull request may close these issues.

Move Slice Splatting to gen_js_to_c
2 participants