-
Notifications
You must be signed in to change notification settings - Fork 36
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
base: master
Are you sure you want to change the base?
Conversation
pub struct Module(*const sys::FizzyModule); | ||
pub struct Module { | ||
ptr: *const sys::FizzyModule, | ||
owned: bool, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
baf8217
to
17b3dca
Compare
3c28438
to
a0c6959
Compare
7923005
to
4f6b02e
Compare
No description provided.