PortableBitfields icon indicating copy to clipboard operation
PortableBitfields copied to clipboard

Fixed an overflow error in field_size

Open KevinNelsonPlexus opened this issue 10 months ago • 4 comments

There is an overflow in field_size which causes a compiler error. This occurs in the special case where a single field is defined which is the same width as the underlying type. Added a check to handle this edge case, and tests to verify.

KevinNelsonPlexus avatar Mar 11 '25 18:03 KevinNelsonPlexus

Sorry, for such a late response. Busy times at work ;)

I understand that there is a compilation error here, but I find having one field only, which occupies the entire bitfield structure, not needed. Actually, it's good that there is a compilation error: maybe we should make the static_assert tell the user to use a "normal" variable.

I might not see a use case here. Could you give an example why the user would like to use the structure for one field occupying the entire structure?

KKoovalsky avatar Jul 01 '25 11:07 KKoovalsky

No worries! I thought I would just contribute what I found.

I end up using it in automatically generated code. In my case, portable bitfields wraps a FPGA register which can sometimes be a single field. Rather than having a different interface for these specific registers, and needing extra complexity in the code generation, I just tweaked the library to allow this case.

KevinNelsonPlexus avatar Jul 02 '25 17:07 KevinNelsonPlexus

I could merge it, but it's missing the runtime tests against at(), extract() and serialize() on one field in the bitfield. There is only one test:

    SECTION("For one field, occupying the whole bitfield")
    {
        Bitfields<uint8_t, Field<Reg::field1, 8>> bf;
        bf.at<Reg::field1>() = 0b10010110;
        REQUIRE(bf.extract<Reg::field1>() == 0b10010110);
    }

https://github.com/KKoovalsky/PortableBitfields/blob/main/tests/test_extracting.cpp#L77

  1. I think we should create a templated-test on types uint8_t, uint16_t, uint32_t and uint64_t for these operations, to confirm they work as expected for one-field bitfield.
  2. Moreover, we would need to support the case when the single field in the bitfield doesn't occupy the entire bitfield. This also requires testing.

Would you be willing to extend the test cases?

Best regards, Kacper

KKoovalsky avatar Jul 03 '25 09:07 KKoovalsky

I suggest adding the static_cast fix from this PR to this PR.

nate-plxs avatar Aug 28 '25 16:08 nate-plxs