feat(#3): Derive FromStr for OpCode using strum dependency #4
Conversation
There was a problem hiding this comment.
Thanks for the PR @Hasenn
Could you please:
- Add an SOB to your commit message,
- Add a commit message, not just a title. Explain why this is needed
- Follow the existing commit subject pattern. Here it should be :
do-core: Instruction: Implement FromStr - Add a
Fixes #3to your commit message in order to link it to the issue.
do-core/Cargo.toml
Outdated
| @@ -5,3 +5,5 @@ authors = ["Samuel Ortiz <sameo@linux.intel.com>"] | |||
| edition = "2018" | |||
|
|
|||
| [dependencies] | |||
| strum = "0.20.0" # enum utils with a derive for FromStr | |||
| strum_macros = "0.20.1" | |||
There was a problem hiding this comment.
This brings 6 new dependent crates.
If we only need the FromStr trait, I would prefer that we implement it instead.
There was a problem hiding this comment.
Ok we can do that, i tried to pick the least evil between those dependencies and failing DRY
There was a problem hiding this comment.
Trying is not failing :) A lot of projects are using strum so it is a good option. But if we need only one struct to implement FromStr, then we're talking about 20 lines of codes or so, vs 6 new crates. Security and maintenance wise, it makes more sense to implement our own. As the golang proverb says: "A little copying is better than a little dependency."
|
Should i |
That's a wildcard that's typically used in test modules, yes. Fine by me. |
|
@sameo Changes done, ready for review |
do-core/src/instruction.rs
Outdated
| "ST" => Ok(OpCode::ST), | ||
| "ADD" => Ok(OpCode::ADD), | ||
| "XOR" => Ok(OpCode::XOR), | ||
| _ => Err(Error::ParseOpError), |
There was a problem hiding this comment.
This needs some formatting love. cargo fmt will help you.
There was a problem hiding this comment.
i ran it earlier and it unaligns everything even more :
"LD" => Ok(OpCode::LD),
"ST" => Ok(OpCode::ST),
"ADD" => Ok(OpCode::ADD),
"XOR" => Ok(OpCode::XOR),
_ => Err(Error::ParseOpError),
Much better. Could you also add you Signed-off-by to the commit message ( |
Related issue: dev-sys-do#3 This is for do-core-asm to be able to depend on us for its opcodes needs Signed-off-by: Eliott Veyrier <eliott.veyrier@umontpellier.fr>
Signed-off-by: Eliott Veyrier <eliott.veyrier@umontpellier.fr>
|
this closes #3 |
With naive pattern matching check. Live exercice #4. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
This will allow my assembler to depend on do-core for its OpCode parsing
I added a dependency to strum and strum_macros to automatically generate the FromStr implementation, which doesn't introduce any overhead to adding new opcodes.