Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encoder identity refactoring changes #19

Merged
merged 7 commits into from
Dec 24, 2024
Merged

Encoder identity refactoring changes #19

merged 7 commits into from
Dec 24, 2024

Conversation

cheng-alvin
Copy link
Owner

This Pull, the encoder module received minor updates and changes including function and enum renaming as well as minor refactoring works carried out. Notably, the lookup table for encoders has been moved from the instruction module over to the encoder module and all encoder functions must be call through this function, encoder functions shall not be called directly. More details appear in the commit logs

This commit renamed all the enocder identitiy enums from the prev-
ious `OP_` prefix to the `ENC_` prefix. Previously, the enocder
identities were placed in the `operand.h` file and held the `OP_`
prefix before it was moved to this file. However despite being moved
to another module, the old name stayed around until it was finally
renamed in this commit.
Previosuly, the enocder function definitions were literally copied
and pasted into a header file and introduced lots of verbosity to
the header, things like the encoder arguments were repeated many
times and was hard to maintain, this commit rectifies this issue
by creating a *temporary macro* that will create function defini-
tions for you automatically using the standard argument library.
Previous name of the common function for the `MR` and `MR` encoder
identities carried the un-descripitive and in conclusive name of
 `mr_rm_ref()`, this new name - `rm_mr_common` should better explain
the funcitonality of this encoder helper function.

Further more, this commit also removed the arbitary boolean param-
eter value that described if the function was used to encode the
`RM` or `MR` encoder identity, now this function simply takes a
enumeration of the current identity and changes behavior based on
this.
This commit migrated some functions and types from the `instruct-
ion.h` header to the new `encoder.h` as well as removing the enocder
function definition macros. Please see the detailed changelog below:

- Since there is *no recursive* preprocessing in the normal C stand-
ard, the previous macro, namely the `DECLARE_ENCODER_LIST` macro
fails to work, therefore removed in favour of the migration of the
`instr_encode_func` from the `instruction.h` header.

- The `instr_encode_func` and `instr_encoder_t` function and type
respectively has been moved over to the `encoder.h` file along with
the renaming of their prefixes to `enc` (Which stands for encoder)
rather than the old `instr` prefix (aka *instruction*)

Now, the enocder functions can be accessed using the `enc_lookup`
wrapper function so the function definition doesn't need to be
declared in the `encoder.h` file and prevents namespace poullution
issues as well as naming clashes.
As per the commit message in 64b99a5:

> - The `instr_encode_func` and `instr_encoder_t` function and type
> respectively has been moved over to the `encoder.h` file along with
> the renaming of their prefixes to `enc` (Which stands for encoder)
> rather than the old `instr` prefix (aka *instruction*)

For somereason this was not carried over for the header definiton,
causing linkage errors during linktime. This commit just matches
this with the function prototype in the source file. (No more linker
issues now :))
After the migration and renaming of the instruction encoder look-
up function in the previous commit, the callers were not adjusted
to the change. This commit adds the `encoder.h` header file instead
(Since it moved) and changed name to match function call
IMO using the enocder type as a type for pre processors seems very
confusing, so this commit defined the `encoder_t` type to another
name sort of like a *alias* as the name of `pre_encoder_t`
@cheng-alvin cheng-alvin added enhancement New feature or request patch Small and minor patch refactor Minor changes in favour for maintainability and/or readability labels Dec 24, 2024
@cheng-alvin cheng-alvin merged commit b386787 into main Dec 24, 2024
4 checks passed
@cheng-alvin cheng-alvin deleted the enoder_ident branch December 24, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request patch Small and minor patch refactor Minor changes in favour for maintainability and/or readability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant