Rust: Refactor Architecture trait to enforce method implementations for associated types at compile-time#7942
Open
mkrasnitski wants to merge 3 commits intoVector35:devfrom
Open
Rust: Refactor Architecture trait to enforce method implementations for associated types at compile-time#7942mkrasnitski wants to merge 3 commits intoVector35:devfrom
Architecture trait to enforce method implementations for associated types at compile-time#7942mkrasnitski wants to merge 3 commits intoVector35:devfrom
Conversation
* Add `UnusedFlagClass` and `UnusedFlagGroup` structs
* Move the following methods from the `Architecture` trait into the
various flag traits and pruned their list of super traits:
* `Architecture::{flags, flag_from_id}` moved into `Flag`
* `Architecture::{flag_write_types, flag_write_from_id}` moved into `FlagWrite`
* `Architecture::{flag_classes, flag_class_from_id}` moved into `FlagClass`
* `Architecture::{flag_groups, flag_group_from_id}` moved into `FlagGroup`
Move `Architecture::{intrinsics, intrinsic_from_id}` into the
`Intrinsic` trait.
Move `Architecture::{register_stacks, register_stack_from_id}` into the
`RegisterStack` trait. Plus, move the following into the `Register` trait:
* `Architecture::registers_all`
* `Architecture::registers_full_width`
* `Architecture::registers_global`
* `Architecture::registers_system`
* `Architecture::stack_pointer_reg`
* `Architecture::link_reg`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, the
Architecturetrait has lots of methods which are documented as required only if a specific feature is supported by the architecture. For example, if an architecture does not use flags, thenArchitecture::Flagcan be set toUnusedFlag. Otherwise, you need to provide a custom type which implementsFlag, and you must also override the default implementations ofArchitecture::flagsandArchitecture::flag_from_id. This is a brittle approach because it's not enforced by the compiler. What this PR does is create corresponding required methods on theFlagtrait which are then simply wrapped by the methods onArchitecture.In total, the following traits are refactored in this way:
FlagFlagWriteFlagClassFlagGroupIntrinsicRegisterRegisterStackAll the methods return some variant of
Self,Option<Self>, orVec<Self>, and are therefore associated methods which do not take&selfas an argument. However, in order for the variousCore*structs (likeCoreFlag,CoreRegister, etc.) to implement these new methods, I had to add anarch: &CoreArchitectureparameter through which the raw handle could be accessed when performing raw API calls into the core. Most custom architectures will not use this parameter so it's slightly less ergonomic this way, but I wasn't able to find a workaround. Maybe generics could help here?Other miscellaneous things of note:
UnusedFlagClassandUnusedFlagGroupstructs which reduce boilerplate for custom architectures with simple flag implementations.Register::registers_full_widthto no longer be required by having it callRegister::registers_allby default, and added a doc-comment that it should be overridden if the architecture has any non-fullwidth registers.