MSVC warnings

[christianrauch]

Enable warnings for Microsoft Visual C++.

[christianrauch]

@humanoid2050 The warnings given by MSVC return a signed comparison of unsigned values: msp\inc\msp\bytevector.hpp(426): warning C4146: unary minus operator applied to unsigned type, result still unsigned

Since count and offset are both of type size_t, wouldn't the following comparison:

if(count < -offset) return false;

always be true, i.e. return false, if offset>0?

Btw: I undid the explicit cast for value to make its value access less verbose.

[humanoid2050]

I could have sworn size_t was signed in my system, but I'm using linux/gcc. That particular line of code was so that bytes could also be "unconsumed" for reprocessing. It's not used that way anywhere as I recall, but it has the capability.

[christianrauch]

size_t is used to represent the size of arrays or memory: https://en.wikipedia.org/wiki/C_data_types#stddef.h

It appears to me that offset is used as unsigned value throughout ByteVector.

Should offset be signed or should this comparison be removed?

[humanoid2050]

The input should be signed, and the comparison needs to happen so that the user doesn't unconsume more bytes than have already been consumed. Not sure off the top of my head the best solution. Maybe if (count < 0 && abs(count) > offset) return false

[christianrauch]

As I understand, consume() is just for unpacking bytes without using them. Hence, the only constrain is that we cannot consume more bytes as there are left, i.e. we cannot consume more than size-offset. I am simply reusing unpacking_remaining(), which returned the unsigned / absolute difference of size and offset. Hence, this assumes that offset cannot get larger than size.

The warnings revealed another issue: Some messages use boolen flags internally, which are incompatible with the underlying byte storage of ByteVector. E.g., the DataflashSummary (MSP_DATAFLASH_SUMMARY) used a bool to store flash_is_ready. After looking into the betaflight code (https://github.com/betaflight/betaflight/blob/72d01172783ca28ac1892851291fb87c72dc5d9f/src/main/msp/msp.c#L338) this is supposed to be a byte for flags, and flashfsIsReady is only one bit of it. There are other cases where only a bit is required, but a byte is transferred and the warning is complaining because a bit is serialised into a byte. ~Unless there is a way to automatically upcast bool to uint8, the simplest workaround is currently to define these flags as byte.~

I am mainly using Linux/gcc too, but enabling these warnings for MSVC on the CI uncovers more potential issues.

[humanoid2050]

The use of the function can be restricted to moving the offset forward instead of forward and back. That would solve the problem by changing the definition of what the function does.

I imagined a scenario where someone might need to do a test unpack of one or more bytes, then unpack them again based on the results of the first unpack.