riscv-isa-sim
riscv-isa-sim copied to clipboard
Teach cfg_t to "freeze" and add isa parsing to it
Split out from #938
The idea here is that we want to compute some "derived" configuration
in cfg_t
, which can depend on more than one of the config arguments.
Specifically, we've got an isa_parser_t
that we want to construct
based on the "isa" and "priv" strings.
One way to do that would be to parse the strings on the fly in some sort of getter function. A problem there is that we'd end up parsing them multiple times (on each call to the getter).
To avoid that re-computation, we could add a local cache. But that's a
bit awkward too: we'd like to pass a "const cfg_t *
" around, but then
we'd have to mess around with const_cast
s to allow us to mutate the
cache. What's more, it would probably be better to do the parsing
at a known time so that we can generate any error messages at a time
we expect.
The "proper RAII way" to do this is probably to define some expanded
version of cfg_t
, whose constructor does all the parsing. Then we'd
pass that expanded configuration around afterwards. Here, we do
something slightly more lightweight and just expect users not to alter
any of the (public) configuration arguments once they've called
freeze()
.
@scottj97: Passing the const object is essentially what we're doing already. The bool frozen
field is just checking that we didn't mess up in spike.cc
and forget to call the freeze method.
You're right that we could use mutable
here, but I'm still not a big fan: the isa_parser_t
constructor can fail (e.g. if the ISA string is bogus) and it feels cleaner to me to know when exactly we're calling it.
I'm happy to remove the frozen status and just leave it up to the callers. Or I'm happy to do the "frozen_cfg_t" class approach. But it looks like you and @aswaterman slightly disagree, which makes it difficult to know what's best to do :-D
Any thoughts?
@scottj97's suggestion is also fine with me: it renders my suggestion moot, so you can ignore mine.
I think what I was picturing before all your recent work is a cfg_t
class that could merge two instances of itself, creating a new merged cfg_t
object. We would construct one from the cmdline, one from the device tree, then merge them.
Or perhaps there's a third one built first with all the default values? Then merge in the cmdline, then merge in the device tree. The tricky part will be dealing with conflicts, but I suspect this will simplify things versus the current direction.
Ideally, each object would be immutable after construction. Certainly, the merged object would be immutable after construction. This way you could do everything at a well-defined time in the constructor.