refactor(crate): refactor robustone-core

[Polardist]

• Use thiserror crate for better error handling and display format
• Refactor architecture for graceful converting &str and String to architecture enum
• Refactor architecture and prepare to deprecated the old ArchitectureUtils usages
• Refactor Instruction inner detail type, remove needless Box operation for dyn trait

The robustone-core crate fmt checks, clippy checks and tests all passed.

No user facing changes.

[Polardist]

CC @Plucky923 , more on that, we have a trait signature

pub trait InstructionDetail: std::fmt::Debug + Send + Sync {
    /// Returns the name of the architecture that produced this detail.
    fn architecture_name(&self) -> &'static str;

    /// Returns a list of register identifiers that are read by this instruction.
    fn registers_read(&self) -> Vec<u32>;

    /// Returns a list of register identifiers that are written by this instruction.
    fn registers_written(&self) -> Vec<u32>;
}

the return Vec always dose clone, change API to return &Vec and let user to determine when to clone could be more graceful. Is it designed to clone every time in purpose ?

[Plucky923]

After the refactoring, ArchitectureUtils seems to be a redundant abstraction. Consider removing it and only provide some auxiliary functions.

[luojia65]

Provide fn normalize_name and fn is_address_aligned at module level; this ArchitectureUtils structure can be removed.

[Polardist]

Provide fn normalize_name and fn is_address_aligned at module level; this ArchitectureUtils structure can be removed.

Do we have to keep normalize_name function, rather than str/String.to_arch() ? Current API provide Architecture from both &str and String.

[Plucky923]

CC @Plucky923 , more on that, we have a trait signature

pub trait InstructionDetail: std::fmt::Debug + Send + Sync {
    /// Returns the name of the architecture that produced this detail.
    fn architecture_name(&self) -> &'static str;

    /// Returns a list of register identifiers that are read by this instruction.
    fn registers_read(&self) -> Vec<u32>;

    /// Returns a list of register identifiers that are written by this instruction.
    fn registers_written(&self) -> Vec<u32>;
}

the return Vec always dose clone, change API to return &Vec and let user to determine when to clone could be more graceful. Is it designed to clone every time in purpose ?

I think it's like this. There should be no redundant heap allocation here. We will consider optimizing it later.

[manchangfengxu]

CC @Plucky923 , more on that, we have a trait signature

pub trait InstructionDetail: std::fmt::Debug + Send + Sync {
    /// Returns the name of the architecture that produced this detail.
    fn architecture_name(&self) -> &'static str;

    /// Returns a list of register identifiers that are read by this instruction.
    fn registers_read(&self) -> Vec<u32>;

    /// Returns a list of register identifiers that are written by this instruction.
    fn registers_written(&self) -> Vec<u32>;
}

the return Vec always dose clone, change API to return &Vec and let user to determine when to clone could be more graceful. Is it designed to clone every time in purpose ?

According to what you said let user to determine when to clone.., can we use Cow?

A clone-on-write smart pointer.

The type Cow is a smart pointer providing clone-on-write functionality: it can enclose and provide immutable access to borrowed data, and clone the data lazily when mutation or ownership is required. The type is designed to work with general borrowed data via the Borrow trait.

[Plucky923]

CC @Plucky923 , more on that, we have a trait signature

pub trait InstructionDetail: std::fmt::Debug + Send + Sync {
    /// Returns the name of the architecture that produced this detail.
    fn architecture_name(&self) -> &'static str;

    /// Returns a list of register identifiers that are read by this instruction.
    fn registers_read(&self) -> Vec<u32>;

    /// Returns a list of register identifiers that are written by this instruction.
    fn registers_written(&self) -> Vec<u32>;
}

the return Vec always dose clone, change API to return &Vec and let user to determine when to clone could be more graceful. Is it designed to clone every time in purpose ?

According to what you said let user to determine when to clone.., can we use Cow?

A clone-on-write smart pointer.

The type Cow is a smart pointer providing clone-on-write functionality: it can enclose and provide immutable access to borrowed data, and clone the data lazily when mutation or ownership is required. The type is designed to work with general borrowed data via the Borrow trait.

There is no need to use Cow. Pass references or ownership directly. These contents are usually allocated and stored in memory. There's no place where they need to be copied.

[luojia65]

This partitioning method doesn't show the number of digits in each field, so I'd recommend leaving it unchanged.

[Polardist]

If leave it unchanged, clippy raise a warning

warning: digits of hex, binary or octal literal not in groups of equal size
   --> robustone-core/src/riscv/shared/encoding.rs:478:28
    |
478 |          let instruction = 0b0000000_00011_00010_000_00001_0110011;
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider: `0b0000_0000_0011_0001_0000_0000_1011_0011`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#unusual_byte_groupings
    = note: `#[warn(clippy::unusual_byte_groupings)]` on by default

I would like to follow the clippy recommandation :)

[Plucky923]

clippy's inspection of binary numbers usually requires splitting every four digits. However, in this context, each split represents each part of the instruction, such as opcode, destination register, etc. Both methods are acceptable, depending on which one is more readable.

[Polardist]

I see it now, I will revert these digital format changes for this PR.

[Plucky923]

LGTM！