search

sparcians / mavis

RISC-V Decoder
Apache License 2.0
1 stars 6 forks source link

Refactoring Form Classes to Reduce Code Duplication #7

Closed kathlenehurt-sifive closed 6 months ago

kathlenehurt-sifive commented 8 months ago

The goal of this refactoring effort was to reduce the size of impl/Form.h by removing the duplicate methods defined in every Form class (getName(), getField(), getOpcodeFields(), print(), etc.).

Firstly, several classes have been renamed:

The file impl/Form.h has been split into several new files in impl/forms. I also renamed mavis/FormIF.h to mavis/Form.h.

All Form classes now only contain public static members (see the Form_AMO class in imps/forms/CommonForms.h for an example). These methods have been implemented in the Form class in mavis/Form.h. Without any static methods, there are some places where direct access to these static members is required, mainly in the Extractors. For example, calls to Form_AMO::getName() have been replaced with Form_AMO::name.

Next, I'd like to look at reducing code duplication in the Extractor classes, but that will come in a later PR.

kathlenehurt-sifive commented 8 months ago

I made this a draft so it doesn't get accidentally merged before I get some feedback on it.

klingaard commented 8 months ago

I might have to duck and run after suggesting this... :laughing: What if the Form implementations are in a .cpp and the header just has the basic static const definitions? I.e. drop the inline and reduce the forms files a LOT -- might speed up compile time dramatically.

kathlenehurt-sifive commented 8 months ago

I might have to duck and run after suggesting this... 😆 What if the Form implementations are in a .cpp and the header just has the basic static const definitions? I.e. drop the inline and reduce the forms files a LOT -- might speed up compile time dramatically.

That's good idea! I'll do that.

kathlenemagnus commented 6 months ago

To be able to move the Form implementations into source files, I'll also need to do the same for the Extractor classes. However, I'd like to refactor the Extractor classes to reduce code duplication first.

I'll open a new PR to refactor the Extractor classes and move the Form and Extractor class implementations into source files.

kathlenemagnus commented 6 months ago

Actually, I figured out how to move the Form implementations into source files. I just needed to remove an unused member (func_fields_) of the Extractor classes. I also went ahead and moved the ExtractorForms and ExtractorDerived files the new impl dir to prepare for more refactoring. The Mavis test runs and passes.

@klingaard and @bdutro-sv this PR is ready to go. I'd like to add a rule to require PRs to be approved by one of the three of us. What do you think?

klingaard commented 6 months ago

CC: @bdutro (he's no longer at SiFive) :grin:

klingaard commented 6 months ago

@jeffnye-gh -- you might be interested in looking at this one.