dolphin-emu/dolphin — pull request #157 by neobrain (2014-03-10 15:59:00+00:00)
Title: [RFC] Bitfield cleanup

### Introduction
This branch provides a generic BitField class for accessing bitfields in an endianness-independent way without sacrificing performance or having to introduce crazy stuff like automated register definition header generation build steps. Details on usage of this class are written in the header file BitField.h. As an example, I have changed two structures in BPMemory.h to use the BitField class.

I repeatedly came across the need for endianness independence in the Dolphin register definitions when writing tools running on the Wii, e.g. native fifoplayer and the GX hardware tests. It would have been pointless to rewrite all the register structures just to have them work on big endian systems, so I was looking for a solution to replace Dolphin's register structures with without changing anything about the regular Dolphin build.

### Technical background
On big endian, the declaration order of bitfields needs to be reversed compared to little-endian. If we had a register definition like

union SomeStructure
{
u32 hex;
struct
{
u32 some_bitfield : 4;
u32 some_second bitfield : 4;
};
};

in Dolphin which I'd want to use on big-endian systems, it would hence need to be rewritten as

union SomeStructure
{
u32 hex;
struct
{
#ifdef LITTLE_ENDIAN
u32 some_bitfield : 4;
u32 some_second bitfield : 4;
#else // if BIG_ENDIAN
u32 : 28; // padding
u32 some_second bitfield : 4;
u32 some_bitfield : 4;
#endif
};
};

Whereas the new BitField class allows it to be written as

union SomeStructure
{
u32 hex;

// no nested structure necessary

BitField<0,4,u32> some_bitfield;
BitField<4,4,u32> second_bitfield;
};

### Pros & Cons
Pros:
* Register definitions with BitField can be used on a wider range of systems due to endianness independence
* Cleans up code because there's no need for nested structures within unions anymore

Cons:
* Slightly more typing required for each line: Instead of `u32 bf : 3;` we need to write `BitField<0,3> bf;`.
* Redundant data: With plain bitfields, the position offset within the raw register value follows from the order of the bitfield declaration (e.g. when writing `u32 a : 2; u32 b:2; u32 c:2` on little-endian, it is implied that a is at the least significant bits, while b is two bits further to the right, etc). This may or may not increase the number of bugs caused by sloppy template parameter specification, but it can also be seen as a Pro because may (or may not) decrease the number of bugs caused by incorrect bitfield padding.
* Requires more careful usage in the context of non-typesafe functions (e.g. printf): In particular, passing BitField objects to printf yields unexpected behavior unless an explicit cast to (u32) is performed. This is not too bad though since at least gcc throws a warning about it.

Non-Cons:
* Performance: I have not verified for the submitted code if the generated assembly code is equal to plain bitfields, however @shuffle2 and I have verified that both the Visual Studio compiler and gcc are able to optimize the used algorithm /in principle/. I doubt the changes I have done since then changed anything about this.

### What to review
There are a number of things I'd appreciate feedback on:
* Anything you would complain about if this was a regular PR
* Implementation: I'm quite happy with the current implementation, since it's able to handle signed and unsigned bitfields in the same class (this used to be different in an earlier prototype). However, if you see any room for improvement I don't mind implementing it. Maybe @shuffle2 and @Billiard can take a quick look over this again?
* Robustness: The class has proven very intuitive and straightforward to use in my workflow. However, I'm not sure if I'm overlooking some critical scenario where people unfamiliar with this class might misuse it such that hard to debug bugs arise. The note about non-typesafe functions might be an example, but I think that's easy enough to recognize and fix due to compilers warning about this.

Note that if we agree on having this in Dolphin (which would make my writing of Wii applications a lot easier) I will take care of writing proper unit tests, but at this stage it has been proven enough to work for me to be confident that it works correctly.

### Design notes
There is some unusual usage of C++ going on in the BitField class. As far as I know, none of it relies on C++ implementation specific behavior, so it should all be legal:

* BitField constructors: I've deleted some constructors which might be used improperly (in particular BitField(u32), as pointed out in the class definition). On the other hand, the constructor BitField() is required because sometimes people want to construct the contents of e.g. a `UPE_Copy` register from scratch. However, to be able to put BitFields into a union, they must use the default constructor. Hence, BitField() explicitly uses a default constructor.
* Each bitfield defines its own storage: However, since all bitfields are put into a single union with the "main" value (e.g. `u32 hex;`), only a single `u32` is allocated. Note that using a reference would work too, but that has the critical drawbacks that it's not possible to have a parameter-less constructor then (the source u32 value would always need to be passed as a constructor parameter).