Add bit_view support class
#1708
Conversation
|
Todo:
Edit: done. |
|
Similar concepts have been tried. For example, this approach uses lots of preprocessor macros to build up a safe union and class members: BEGIN_BITFIELD_TYPE(Status, uint32_t)
ADD_BITFIELD_MEMBER(readers, 0, 10)
ADD_BITFIELD_MEMBER(waitToRead, 10, 10)
ADD_BITFIELD_MEMBER(writers, 20, 10)
END_BITFIELD_TYPE()To me, this is an eye-sore that says "we had to hack the language with macros". Even if it gets the job done, it feels way too much like a custom contraption. There's also this approach, which adds some C++ meta programming o get rid of the macros: union manually_done_bitfield {
using element_t = long;
using base_t = base<long, 4, 3, 5>;
BitFieldMember<element_t, base_t::displacement(0), 4> fourBits;
BitFieldMember<element_t, base_t::displacement(1), 3> threeBits;
BitFieldMember<element_t, base_t::displacement(2), 5> fiveBits;
};However, these |
kcgen
added
the
plumbing
UBSAN testsBuildmeson setup -Dbuildtype=debug -Db_sanitize=undefined build/clang-undefined
meson compile -C build/clang-undefinedResults |
|
Looking nice. From an ergonomics point of view, is there a way of creating the bit_view without explicitly setting the type? I know C++ can do some magic regarding templates and type inference, but unsure of when/how it can do so. |
|
@shermp , yeah, the position and width of the bit_view can give you the largest type needed for that specific bit_view. So that means, say, for a 16bit register, the first handful of bit_views would use uint8_t and the latter handful would use a uint16_t. The mixed-sizing made me nervous enough to try to avoid it, and make sure all the bit_views are aliasing /holding the same data-type. (But that's perhaps an unecessary concern!) Like you're suggesting, ideally there's some introspection that can tell us the size of the parent structure, and figure out that we're sitting inside a union 16-bits in size, then everything could use that. Something like that would get it to the 100% level. I also like that this approach would prevent use of the wrong type (although the static_asserts guarantee the bit_view won't go out of range). It's also DRYer: you'd drop in the single data type, and then everything else uses that without any repetition. |
We're using the bit-views for low-level stuff only, and in that context the register/data widths are fixed and known in advance in 99.99% of the cases. So I very much prefer the current approach of having to specify the width explicitly for each register or data type we're dealing with; that's much more descriptive, reflects actual reality, and is a lot less error prone. |
|
If we can lock'em in: union MyReg {
uint8_t data; // this is the type we want to learn
bit_view<...> // about and lock-in for all the peer bit_views
};I've been scanning the web but can't find how to get the data type from the parent union/class (without macro-rewrite hacks). Might have to settle for manually repeating it? |
|
Nice addition @kcgen, this will make all that low-level bit-twiddling less error-prone and more readable! 👍🏻
If there was a simple way to avoid the manual repetition that would be preferable, of course, but I don't think it's worth complicating the code with lots of template/macro magic just to make that happen somehow. |
"bit_view" is a better description for how the class operates given it's a "view" of the bits held in the the underlying storage. This is meant to be similar to "string_view", which is also a similar "functional wrapper" around the underlying char* data. --- The author of the BitField class also describes it like this: Q. How do you ensure all the bitfields are usable? Because inherently, union will only use the max size member. You will not be able to see unique data for other members, if one is assigned a value, it will be the value for all other members. A. Remember that this isn't actually using bit-fields, but is emulating the effect of them. The "bits" are just a "view" into the underlying storage. Changing one is intended to be reflected in both. --- Other changes: - Use lower-case types and name similar to 'string_view', given there's nothing DOSBox-specific about it. This makes it feel more like "standard plumbing", and indeed, anyone can take this class into other projects - Add compile-time asserts to catch index and width issues - Make all functionality constexpr (compile-time evaluation) - Add unit tests for all functions - Fix decrement bug caught in unit tests - Fix comparison bugs caught in unit tests - Drop single-bit template specialization. A bit_view with a size of 1 is already known and optimized at compile-time now using "constexpr" - Remove the bool cast operators in favour of exact value operator, which are clearer to understand - Add member functions from bitops: - clear(): sets bits to zero - flip(): flip bits - none(): check if all bits are unset - any(): check if at least one bit is set - all(): check if all bits are set - Add comments - License as GPL v2+ (from public domain), given all lines have changed. Credit and thanks given to Evan Teran (thanks!)
|
It seems GitHub has pushed an update to the macOS runners, however it's trapped my runners in a crash loop: Will try to get it sorted tomorrow. |
|
Fleet back online 🚢
|
|
@kklobe , @Wengier , @shermp: GitHub recently released native ARM64 builds of their runner, which the previous x64-on-rosetta2 runners couldn't auto-upgrade to. So I had to start with a clean slate. If you still have ARM-based runners using the x86-binaries, drop me a note and I'll pass the new setup steps (which include the project token). |
|
Thanks for the review @shermp and @johnnovak.
@shermp, your suggestion is now addressed safely and with compile-time asserts; so it's impossible to read or write before or after the view's data value. Unit tests are passing on all platforms, along with passing all the *SAN tests locally. Merging. |
The
bit_viewclass is a wrapper around a data value. It provides a view into a subset of its bits allowing them to be read, written, assigned, flipped, cleared, and tested, without the need to for the usual twiddling operations.Constructing a
bit_viewis similar to C bitfields, however unlike C bitfields,bit_views are free from undefined behavior and have been proven using GCC's and Clang's undefined behavior sanitizers.This gives us the benefit of bitfields without their specification downsides:
constexpr)bit_viewis self-documenting by specifying its bit index, number of bits, and logical nameBefore and after comparison
Assume we have a 16-bit audio control register that holds the card's:
The existing DOSBox code would ferret out this information using manual bit twiddling:
The
bit_viewmakes the register's logical elements self-documenting:The views can be used directly, making for much clearer (and safer) code:
AudioReg reg = {data}; if (reg.speaker_on) enable_speaker(); else disable_speaker(); const auto left_percent = reg.left_volume / 63.0; const auto right_percent = reg.right_volume / 63.0; set_volume(left_percent, right_percent);