Make clear that parameter indexes are InstIdxs.
#1484
Conversation
ltratt
force-pushed
the
parameter_indexes
branch
from
e4b98ee
to
f4468c8
Compare
| pub(crate) fn params(&self) -> &[yksmp::Location] { | ||
| &self.params | ||
| /// Return the parameter at a given [InstIdx]. | ||
| pub(crate) fn param(&self, iidx: InstIdx) -> &yksmp::Location { |
There was a problem hiding this comment.
I'm not sure about making this InstIdx. My understanding is that an InstIdx is only used to index into the insts vector of the module. Should this not be a new type ParamIdx, as it's indexing into the params vector?
There was a problem hiding this comment.
Interestingly, I originally made it ParamIdx, got almost as far as a PR then.... I realised that the if we have n parameters, the first n instructions must be parameters. It's baked into our system that %0...%n are ParameterInsts, so introducing ParamIdx meant I just ended up with (infallible) conversions to/from InstIdx without really doing much useful.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Previously we used a `u32` (why such a big type? dunno!), requiring
`ParamInst` to be `packed`. By definition a parameter is stored in an
SSA variable which, in our context, means it's an `InstIdx`.
This commit makes this explicit:
1. Parameters are now indexed by `InstIdx`.
2. The vague `locidx` is now `paramidx`.
3. The unnecessary largesse of the `params()` function returning a
slice is hidden by a `param()` function which returns a single
parameter.
This change did spot a bug in a test, so it is useful.
|
Force pushed an update which fixes that Clippy warning. Should hopefully merge now. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
ltratt
force-pushed
the
parameter_indexes
branch
from
f4468c8
to
2161584
Compare
|
Oops, I didn't actually force push. Now I have. Hopefully it'll merge now! |
Previously we used a
u32(why such a big type? dunno!), requiringParamInstto bepacked. By definition a parameter is stored in an SSA variable which, in our context, means it's anInstIdx.This commit makes this explicit:
InstIdx.locidxis nowparamidx.params()function returning a slice is hidden by aparam()function which returns a single parameter.This change did spot a bug in a test, so it is useful.