Remove to_headers
#184
Remove to_headers
#184
Conversation
MarquessV
changed the base branch from
main
to
1455-python-support-for-program-and-beyond
| @@ -250,43 +251,40 @@ impl Program { | |||
| Ok(expanded_program) | |||
| } | |||
|
|
|||
| pub fn to_instructions(&self, include_headers: bool) -> Vec<Instruction> { | |||
| let mut result = vec![]; | |||
| pub fn to_instructions(&self) -> Vec<Instruction> { | |||
There was a problem hiding this comment.
As long as we're breaking here ... should this take self? That's my understanding of the convention with to_x, @Shadow53 wdyt
(For later consideration: I suppose if we want a borrowed equivalent there could be an iter_instructions which chains these all together and thus doesn't need a clone)
There was a problem hiding this comment.
to_ is actually the correct prefix here, according to the Rust API naming guidelines
(Summary: as_ == cheap borrowed, to_ == expensive operation, likely with cloning, into_ == owned-to-owned conversion)
There was a problem hiding this comment.
I do like the idea of a method returning an iterator. Per convention, it would probably just be called instructions().
There was a problem hiding this comment.
For my own learning, I tried implementing an instructions() method that returns an iterator but ran into trouble finding a way to do it without cloning. As a slimmed down example, consider:
pub fn instructions(&'a self) -> impl Iterator<Item = &Instruction> + 'a {
self.memory_regions
.iter()
.map(|(name, descriptor)| {
Instruction::Declaration(Declaration {
name: *name,
size: descriptor.size,
sharing: descriptor.sharing,
})
}) // => Returns an Iterator with `Item = Instruction`
.chain(self.instructions.iter()) // Returns an Iterator with `Item = &Instruction`
}Because the types of the iterators are mismatched, Rust won't compile this code. To avoid cloning self.instructions, we have to create an iterator of references to the Declaration instructions, which can't be done with out collecting them into something like a vector first. We'd have to do the same for frames and calibrations as well, since we also build new Instructions out of their data. Maybe there is a workaround for this, but I couldn't find one after playing with it for a bit.
There was a problem hiding this comment.
Only workaround I can think of is to use Cow inside Instruction, which is more refactoring than I think we want to do here.
| @@ -250,43 +251,40 @@ impl Program { | |||
| Ok(expanded_program) | |||
| } | |||
|
|
|||
| pub fn to_instructions(&self, include_headers: bool) -> Vec<Instruction> { | |||
| let mut result = vec![]; | |||
| pub fn to_instructions(&self) -> Vec<Instruction> { | |||
There was a problem hiding this comment.
I do like the idea of a method returning an iterator. Per convention, it would probably just be called instructions().
Co-authored-by: Michael Bryant <mbryant@rigetti.com>
Cargo.lock
Outdated
There was a problem hiding this comment.
knope typically generates and commits this, so I had to bypass the .gitignore to push this update. This updates the crossbeam dependency of criterion to a newer version that hasn't been yanked.
| @@ -250,43 +251,40 @@ impl Program { | |||
| Ok(expanded_program) | |||
| } | |||
|
|
|||
| pub fn to_instructions(&self, include_headers: bool) -> Vec<Instruction> { | |||
| let mut result = vec![]; | |||
| pub fn to_instructions(&self) -> Vec<Instruction> { | |||
There was a problem hiding this comment.
Only workaround I can think of is to use Cow inside Instruction, which is more refactoring than I think we want to do here.
Closes #151
Deleted a bunch of now unreferenced snapshots too