Added explicit type conversion in const at()

[herrfrei]

Needed by new version of Visual Studio 2022 compiler

[KKoovalsky]

Hey! Thanks for contributing! :) Could you provide a small sample of code and the requirements, like how to build the sample and with what compiler invocation, to reproduce?

[meikjaeckle]

I think I have the same issue. You can reproduce the issue with following example code. I used the https://godbolt.org/ online compiler (x86-64 gcc 13.2, Xtensa ESP32 gcc 8.4.0 2021r2 and x86-64 clang 17.0.1 ) with flags "-O3 -std=c++17". All failed to build the example code without the fix from herrfrei.

This is the error message from the clang compiler:

<source>:97:16: error: cannot assign to variable 'result' with const-qualified type 'const value_type &' (aka 'const unsigned char &')
   97 |         result &= non_shifted_field_masks[idx];
      |         ~~~~~~ ^
<source>:240:41: note: in instantiation of function template specialization 'jungles::Bitfields<unsigned char, jungles::Field<BitFields::BIT_7, 1>, jungles::Field<BitFields::BIT_6, 1>, jungles::Field<BitFields::BIT_5, 1>, jungles::Field<BitFields::BIT_4, 1>, jungles::Field<BitFields::BIT_3, 1>, jungles::Field<BitFields::BIT_2, 1>, jungles::Field<BitFields::BIT_1, 1>, jungles::Field<BitFields::BIT_0, 1>>::at<BitFields::BIT_1>' requested here
  240 |         field2.at<BitFields::BIT_0>() = field1.at<BitFields::BIT_1>();
      |                                                ^
<source>:96:15: note: variable 'result' declared const here
   96 |         auto& result{ field_values[idx] };
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Compiler returned: 1

enum class BitFields
{
    BIT_0,
    BIT_1,
    BIT_2,
    BIT_3,
    BIT_4,
    BIT_5,
    BIT_6,
    BIT_7
};

using BitFieldType = jungles::Bitfields<uint8_t,
                                        jungles::Field<BitFields::BIT_7, 1>,
                                        jungles::Field<BitFields::BIT_6, 1>,
					jungles::Field<BitFields::BIT_5, 1>,
					jungles::Field<BitFields::BIT_4, 1>,
					jungles::Field<BitFields::BIT_3, 1>,
					jungles::Field<BitFields::BIT_2, 1>,
					jungles::Field<BitFields::BIT_1, 1>,
					jungles::Field<BitFields::BIT_0, 1>>;

int main()
{
	const BitFieldType field1(0b10);
	BitFieldType field2;

	field2.at<BitFields::BIT_0>() = field1.at<BitFields::BIT_1>();
    
    std::cout << "Field: " << static_cast<uint16_t>(field2.serialize()) << std::endl;
    return 0;
}

[herrfrei]

@KKoovalsky Here is an example. I implemented a simple test case for that:

/**
 * @file        test_const.cpp
 * @brief       Tests handling of const Bitfields
 * @author      Kacper Kowalski - [email protected]
 */
#include <catch2/catch_test_macros.hpp>

#include "jungles/bitfields.hpp"

#include "helpers.hpp"

using namespace jungles;

typedef Bitfields<uint16_t, Field<Reg::field1, 2>, Field<Reg::field2, 8>, Field<Reg::field3, 6>> MyBitfields;

MyBitfields encode(uint8_t f1, uint8_t f2, uint8_t f3)
{
	MyBitfields ret;
	ret.at<Reg::field1>() = f1;
	ret.at<Reg::field2>() = f2;
	ret.at<Reg::field3>() = f3;
	return ret;
}
void decode(const MyBitfields &bf, uint8_t& f1, uint8_t& f2, uint8_t& f3)
{
	f1 = (uint8_t)bf.at<Reg::field1>();
	f2 = (uint8_t)bf.at<Reg::field2>();
	f3 = (uint8_t)bf.at<Reg::field3>();
}

TEST_CASE("Const instance handling", "[const]")
{
    SECTION("Decoding from const instance")
    {
		MyBitfields test = encode(0x2, 0xAB, 0x17);
		uint8_t f1, f2, f3;
		decode(test, f1, f2, f3);
		REQUIRE(f1 == 0x2);
		REQUIRE(f2 == 0xAB);
		REQUIRE(f3 == 0x17);
    }
}

Btw., there is a problem compiling the tests with Visual C++. In the macro CreateRuntimeTests() the compile options must be changed from

macro (CreateRuntimeTests)
    ...
    target_compile_options(jungles_bitfield_runtime_tests PRIVATE -Wall -Wextra)
    ...
endmacro()

to

macro (CreateRuntimeTests)
    ...
    target_compile_options(jungles_bitfield_runtime_tests PRIVATE -Wall)
    if(NOT MSVC)
        target_compile_options(jungles_bitfield_runtime_tests PRIVATE -Wextra)
    endif()
    ...
endmacro()

since Wextra is not supported.

[meikjaeckle]

Any news on this PR?

[KKoovalsky]

Hey, thanks for contribution!

@herrfrei Your solution is good, but I used explicit type, instead of C-like casting: 2bf566841ea7ac9d307de7e6494a9ec329b6c794

Also, instead of:

macro (CreateRuntimeTests)
    ...
    target_compile_options(jungles_bitfield_runtime_tests PRIVATE -Wall)
    if(NOT MSVC)
        target_compile_options(jungles_bitfield_runtime_tests PRIVATE -Wextra)
    endif()
    ...
endmacro()

I have used a generator expression: df5e27ef7cefdebcb4f9a33de887a169960e9840

Thanks once again for contributing, and looking forward to see more ;)

[meikjaeckle]

Hi, the issue is not solved with your changes. Your test cases are missing to verify the method constexpr const UnderlyingType& at() const noexcept. It still fails to compile because you try to get a non-const reference to the array object in a const method. But you need a non-const ref, because of the bitmask operation result &= non_shifted_field_masks[idx];.

One solution would be as suggested by @herrfrei to use const_cast (UnderlyingType result{const_cast<UnderlyingType&>(field_values[idx])};), but this would violate the const correctnes of the method.

Another solution would be to just return a copy instead of the reference:

    template<auto FieldId>
    constexpr const UnderlyingType at() const noexcept
    {
        constexpr auto idx{find_field_index<FieldId>()};
        return (field_values[idx] & non_shifted_field_masks[idx]);
    }

[KKoovalsky]

Hmm, I see, thanks for catching this.

(Thinking out loud) this line result &= non_shifted_field_masks[idx]; is here because of the overflow operation. Maybe we could (1) come up with a different strategy of handling the overflows, or (2) implement variants when at() is used as left-operand/right-operand with const-ref/non-const-ref. I need to rethink that.

In the meantime, as a workaround, is it OK for you @herrfrei @meikjaeckle to use non-const variables and refs?

[KKoovalsky]

@meikjaeckle this example:

enum class BitFields
{
    BIT_0,
    BIT_1,
    BIT_2,
    BIT_3,
    BIT_4,
    BIT_5,
    BIT_6,
    BIT_7
};

using BitFieldType = jungles::Bitfields<uint8_t,
                                        jungles::Field<BitFields::BIT_7, 1>,
                                        jungles::Field<BitFields::BIT_6, 1>,
					jungles::Field<BitFields::BIT_5, 1>,
					jungles::Field<BitFields::BIT_4, 1>,
					jungles::Field<BitFields::BIT_3, 1>,
					jungles::Field<BitFields::BIT_2, 1>,
					jungles::Field<BitFields::BIT_1, 1>,
					jungles::Field<BitFields::BIT_0, 1>>;

int main()
{
	const BitFieldType field1(0b10);
	BitFieldType field2;

	field2.at<BitFields::BIT_0>() = field1.at<BitFields::BIT_1>();
    
    std::cout << "Field: " << static_cast<uint16_t>(field2.serialize()) << std::endl;
    return 0;
}

Is logically incorrect, because this: field2.at<BitFields::BIT_0>() = field1.at<BitFields::BIT_1>(); shouldn't compile by design as one can't mutate a const entity.

I guess you meant something like:

f1 = test.at<Reg::field1>();

Which indeed gives a compilation error. :)

[herrfrei]

Hmm, I see, thanks for catching this.

(Thinking out loud) this line result &= non_shifted_field_masks[idx]; is here because of the overflow operation. Maybe we could (1) come up with a different strategy of handling the overflows, or (2) implement variants when at() is used as left-operand/right-operand with const-ref/non-const-ref. I need to rethink that.

In the meantime, as a workaround, is it OK for you @herrfrei @meikjaeckle to use non-const variables and refs?

As a workaround it's okay. But nevertheless we need a solution to read const Bitfields...

[KKoovalsky]

Reading const Bitfields is now implemented. See https://github.com/KKoovalsky/PortableBitfields/commit/e5af26405af361aaec6aa4b8d2250c36886723f2