sunfishcode opened PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
I think this would probably be best done by keeping
&self
as an argument, because that way if you want to testFunc
a few times you don't have to worry about cloning and getting theFunc
back in case of an error.I think we can still return a
'static
closure here, it just needs to close over theInstanceHandle
as well.
alexcrichton created PR Review Comment:
Out of curiosity, how come this changed? Is the intention to avoid storing the
GlobalType
next to theExportGlobal
since they're basically inferred from one another?
alexcrichton created PR Review Comment:
It looks like a lot of the diff is due to the change here, I'm curious how come this changed though?
alexcrichton created PR Review Comment:
I think if we return an
ExactSizeIterator
we can avoid adding an extra method for this, since it should still be possible to domodule.imports().len()
in that case.
alexcrichton created PR Review Comment:
This may need double-checking, but I think
self.module
may not be necessary any more after this change. I think theStore
is still needed but we may be able to remove theModule
field from anInstance
?
alexcrichton created PR Review Comment:
To enhance this a bit, perhaps this could return an
ExactSizeIterator
which should allow for enumeration and such?
sunfishcode created PR Review Comment:
With
Instance::exports()
computingExtern
values on demand, it now returnsExtern
instead of&Extern
. In code likelet func = instance.get_export("foo").unwrap().func().unwrap()
, iffunc()
took a&self
, it'd be taking a reference to a temporary.
sunfishcode submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Makes sense yeah, could we perhaps add alternative methods for these idioms though? Idiomatically this feels like the correct signature for this function (a by-value perhaps being
into_func(self)
). Perhaps we could addInstance::functions()
and such which return iterators over just those values?
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
I did a quick experiment with this, and
Linker::instance
currently needsInstance::module
. We don't currently store the export names in the instance otherwise.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Ah ok no worries. We could make the internal handle
pub(crate)
so the linker could access it, but it's fine to tackle this later if really necessary.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
This is because
Module::exports
is now giving you results by value instead of by reference, and without this a lot of common scenarios have to do awkward things to avoid taking a reference of a temporary.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
And, it turns out, we can actually make
Instance::exports
provide the names in a reasonable way, which helps theLinker
, and makes the API much nicer as you no longer need tozip
withModule::exports
just to get the names.Now, nothing in
Instance
itself or its users need it to have aModule
. I've left it in for now because it looks like it might unregister some things in itsDrop
, but if we want to sort that out we could drop theModule
after instantiation.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Cool, done.
sunfishcode created PR Review Comment:
Works great.
sunfishcode submitted PR Review.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Makes sense. I've now implemented this.
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Ok, renaming them to
into_func()
.We could add
Instance::functions()
if that's a common use case, but users could also writeInstance.exports().filter_map(|export| export.into_func())
if that's what they want.
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Stray lifetime?
alexcrichton created PR Review Comment:
Instead of having a struct for this could we perhaps return a tuple from the exports iterator?
alexcrichton created PR Review Comment:
I've worried historicaly that this is pretty unwieldy. The previous incantation was sort of just on the line of not motivating me to try to improve things, but this feels like it's pretty unergonomic to document as a pattern to call things. Would you be ok adding more accessors in this PR for
Instance
? For example I think we could make this:instance.get_func("foo")?.get2::<i32, i32, i32>()?;
alexcrichton created PR Review Comment:
You can skip the
struct
type definition here, inside of the closure you can dodrop(&instance_clone)
and just useexport
directly, and that'll capture everything necessary.
alexcrichton created PR Review Comment:
I also don't mind pushing up some new iterators/accessors for
Instance
as part of this PR too
alexcrichton created PR Review Comment:
Similar to above, I think we should keep this as an internal field with a public accessor so we can change the make up of the structure over time.
alexcrichton created PR Review Comment:
I personally don't think we should have public-structs-with-public-fields in the API. They're quite inflexible in terms of future additions as well as changing the makeup of the internals. For example committing to two
String
allocations here I think may be something we want to tweak in the future. We could, for example, have something likeArc<str>
which is cheaply cloneable and contains module/string, along with an index of where to split the string.
alexcrichton created PR Review Comment:
Could the old methods be preserved since they can be useful in their own right too?
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode created PR Review Comment:
Oops, yes. This was from an earlier experiment. Fixed now.
sunfishcode submitted PR Review.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Good idea, I've now added
get_func
etc. accessors and update the examples and docs to use them, and that makes things much cleaner.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
I started cleaning this, and ended up finding it made sense to go ahead and eliminate the allocations. So we now have accessor functions, and they return
& 'module str
.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Also fixed.
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
I'm not opposed, I just haven't found cases where they're useful. For the "I don't know what this is" or "This might be one of a few different things" use cases,
Extern
is just anenum
and you can plain oldmatch
orif let
on it, which read better anyway.With the new
Instance::get_func
accessors, evenExtern::into_func
is a little dubious, because you can usematch
andif let
to do that too, but they are handy when you want to report different errors in cases like this:instance .get_export("_start") .context("expected a _start export")? .into_func() .context("expected export to be a func")? .call(&[])?;
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Ah, cool. I've now done this.
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
I think that this is auto-inferred, but could this be changed to
Export<'me>
?
alexcrichton created PR Review Comment:
I think the
'module self
can just beself
here (keeping the&'module str
ret)
alexcrichton created PR Review Comment:
ditto as above
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode updated PR #1524 from refactor
to master
:
This contains refactorings and API changes split off from #1516.
The main public-facing change here is that
Instance
andModule
now compute their imports and exports on demand rather than holding precomputed arrays, andFunc::get*
return closures which capture anInstanceHandle
so that they hold the instance live dynamically.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Done
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Done
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Done
alexcrichton submitted PR Review.
alexcrichton merged PR #1524.
Last updated: Dec 23 2024 at 12:05 UTC