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

rust: module inspection prototype #728

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

rust: module inspection prototype #728

wants to merge 9 commits into from

Conversation

axic
Copy link
Member

@axic axic commented Feb 5, 2021

No description provided.

pub struct Module(*const sys::FizzyModule);
pub struct Module {
ptr: *const sys::FizzyModule,
owned: bool,
Copy link
Member Author

Choose a reason for hiding this comment

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

So I think these are the options for having inspection on the module (which is still available through an instance):
a) either we have this owned flag here and disallow instantiation of a non-owned module
b) introduce a ModuleInspection or some other abstraction which has it, and that can be obtained as module.inspect() or instance.inspect() (this can be seen in one of the earlier commits)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This look ok, although fragile. And I'd rather name the flag instantiated so it is clear it can be instantiated once.

Can't Rust magic memory annotations handle this?

Copy link
Member Author

@axic axic Feb 5, 2021

Choose a reason for hiding this comment

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

Can't Rust magic memory annotations handle this?

One workaround, where I maintain an "instance of module" in the instance and return the reference on it. Since instantiate() requires to own the input, a reference can't be passed so it will not compile.

i.e.

struct Instance {
  instance: NonNull<sys::FizzyInstance>,
  module: Module,
}

impl Instance {
    pub fn get_module(&self) -> &Module {
       &self.module
    }
}

// Then this will be a compilation error: instance.get_module().instantiate() 

Added nastyness is that in instantiate() need to drop module from being managed (i.e. core::mem::forget(instance.module)), otherwise we end up with the same problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessary to save module in Instance, you can call fizzy_get_instance_module

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 downside of the ModuleInspection option is that need to assign its lifetime bound to the underlying Module/Instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary to save module in Instance, you can call fizzy_get_instance_module

That is what is being done in this PR. For that one needs to extend the Module as in this PR.

An alternative approach is to save the module in the instance and return it as a reference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I imagined I guess some combination of your approaches: don't change Module, but have Instance::get_module(&self) -> Module that will call fizzy_get_instance_module, wrap in Module and call core::mem::forget before returning it.

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #728 (4f6b02e) into master (3581284) will decrease coverage by 0.09%.
The diff coverage is 79.24%.

@@            Coverage Diff             @@
##           master     #728      +/-   ##
==========================================
- Coverage   99.27%   99.18%   -0.10%     
==========================================
  Files          88       88              
  Lines       13296    13332      +36     
==========================================
+ Hits        13200    13223      +23     
- Misses         96      109      +13     
Flag Coverage Δ
rust 97.26% <79.24%> (-1.22%) ⬇️
spectests 89.92% <ø> (ø)
unittests 99.22% <ø> (ø)
unittests-32 99.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bindings/rust/src/lib.rs 97.53% <79.24%> (-1.24%) ⬇️

This was referenced Feb 5, 2021
@axic axic force-pushed the rust-module-inspect branch from baf8217 to 17b3dca Compare February 12, 2021 17:02
@axic axic force-pushed the rust-module-inspect branch 4 times, most recently from 3c28438 to a0c6959 Compare May 28, 2022 17:06
@axic axic force-pushed the rust-module-inspect branch from 7923005 to 4f6b02e Compare June 20, 2022 19:18
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