Closed krazy-j closed 1 month ago
Thanks for raising the issue. I can see where you're coming from, and avoiding the type parameter is definitely compelling. That said, I'm still on the fence on this, because logically the CPU is the main entrypoint into the emulator and I like that it's always the same "interface" and you just have to specify the variant as a parameter (Nmos6502
in this case).
I also like that the Variant
trait only has a single method at the moment. The less methods Rust traits have, the more powerful the abstraction.
Perhaps we could add some helper methods like CPU::new_nmos(Memory::new())
or CPU::nmos(Memory::new())
as a compromise? I'm a little worried that the number of constructors might go out of hand, though.
@omarandlorraine, any thoughts?
I also like that the Variant trait only has a single method at the moment. The less methods Rust traits have, the more powerful the abstraction.
This is a good point, but it depends on if we can envisage:
CPU
to be generic across Variant
Variant
trait but which may not be used as a CPU variantthis may be necessary for disassemblers or something in the future
What about a module containing functions for instantiating the most common combinations?
mod new_cpus {
fn typical_nmos<M: crate::memory::Bus>(memory: M) -> crate::cpu::CPU<M, Nmos> {
...
}
fn nmos_thing_with_commodore64_bankswitching<M: crate::memory::FancyBus>(memory: M) -> crate::cpu::CPU<M, Nmos> {
...
}
fn sally<M: crate::memory::AtariBus>(memory: M) -> crate::cpu::CPU<M, Nmos> {
...
}
fn typical_cmos<M: crate::memory::Bus>(memory: M) -> crate::cpu::CPU<M, Cmos> {
...
}
}
just an idea
The longer I think about it, the more I'm of the opinion that the original syntax that we support right now is fine. I mean, except for the needless_pass_by_value
I don't see any big issues. We can revisit that in the future if the number of variants explodes or once we know more about how we want to alter the trait. Sounds acceptable? 😅
That's entirely fair. It was just a suggestion, after all.
Thanks for the understanding and also thanks for raising the question. :)
I noticed the
needless_pass_by_value
on theCPU::new
method, and I wanted to suggest adding a method toVariant
.This would allow constructing
CPU
without the need to use a type parameter or pass the variant as a value. I could create a pull request if that would be helpful.