oxidecomputer / amd-apcb

AMD Generic Encapsulated Software Architecture Platform Security Processor Configuration Block manipulation library
Mozilla Public License 2.0
13 stars 1 forks source link

Remove code duplication in tokens_entry #55

Closed daym closed 2 years ago

daym commented 2 years ago

wesolows wrote:

Is it really necessary to have a separate impl of token() and iter() for TokensEntryBodyItem based on nothing more than the mutability of its type parameter?

daym commented 2 years ago

Is it really necessary to have a separate impl of token() and iter() for TokensEntryBodyItem based on nothing more than the mutability of its type parameter?

I've tried several versions to avoid copying that source block, but it ends up being difficult because there is a conversion from u8 slice to TOKEN_ENTRY reference going on eventually (see list below), making me unable to specify a type for the reference to TOKEN_ENTRY that has the same mutability as the u8 slice used has if TokensEntryItemMut and TokensEntryItem become unified (with type parameter). Because TokensEntryBodyItem<BufferType> is already unified, it then would have to figure out what struct type to pass to the hypothetical UnifiedTokensEntryItem--the easiest way would be having two different iter methods (with differing return types), one for impl TokensEntryBodyItem<&[u8]> and one for impl TokensEntryBodyItem<&mut [u8]>--which is what we wanted to avoid in the first place.

Conversions:

It would be possible to use LayoutVerified<_, [TOKEN_ENTRY]> (note the brackets) instead, but then the token value setters cannot ensure a value range appropriate for the AMD data type (one of {Dword, Word, Byte, Bool}) anymore (because TokensEntryItem then would not exist, thus the AMD data type would be unknown at setter time).

Since the actual user-level token value setters are generated by a macro and are a wrapper above that (which gets a Rust-typed value, so value validity is guaranteed by Rust anyway), that might be fine--but I have to think about it. In short, I'd like to do that in a future PR.

I'll try again--but in my opinion the result will be way more convoluted than the current copy&paste solution, and probably require https://github.com/rust-lang/rust/issues/8995 . On the other hand, I could clean up a LOT of amd-apcb's iterators if it ends up working.

daym commented 2 years ago

I have figured out a way that preserves the functionality but cuts down on the source code. Will land after https://github.com/oxidecomputer/amd-apcb/pull/52 (because it's based on it)