From 83fa4223388487a2388c326cc46cf865198423dd Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Tue, 5 Nov 2024 19:45:49 -0500 Subject: [PATCH] [wpiutil] DynamicStruct: Fix decoding of signed integers (#7350) Add tests for both C++ and Java. --- .../native/include/wpi/struct/DynamicStruct.h | 12 +- .../first/util/struct/DynamicStructTest.java | 83 +++++++++++--- .../native/cpp/struct/DynamicStructTest.cpp | 106 +++++++++++++++--- 3 files changed, 170 insertions(+), 31 deletions(-) diff --git a/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h b/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h index 36d323e625f..c7f64553d45 100644 --- a/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h +++ b/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h @@ -423,7 +423,17 @@ class DynamicStruct { int64_t GetIntField(const StructFieldDescriptor* field, size_t arrIndex = 0) const { assert(field->IsInt()); - return GetFieldImpl(field, arrIndex); + uint64_t raw = GetFieldImpl(field, arrIndex); + switch (field->m_size) { + case 1: + return static_cast(raw); + case 2: + return static_cast(raw); + case 4: + return static_cast(raw); + default: + return raw; + } } /** diff --git a/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java b/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java index bd7697c9e8d..376ed803e65 100644 --- a/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java +++ b/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java @@ -328,21 +328,40 @@ void testDuplicateFieldName() { private static Stream provideSimpleTestParams() { return Stream.of( - Arguments.of("bool a", 1, StructFieldType.kBool, false, false, 8, 0xff), - Arguments.of("char a", 1, StructFieldType.kChar, false, false, 8, 0xff), - Arguments.of("int8 a", 1, StructFieldType.kInt8, true, false, 8, 0xff), - Arguments.of("int16 a", 2, StructFieldType.kInt16, true, false, 16, 0xffff), - Arguments.of("int32 a", 4, StructFieldType.kInt32, true, false, 32, 0xffffffffL), - Arguments.of("int64 a", 8, StructFieldType.kInt64, true, false, 64, -1), - Arguments.of("uint8 a", 1, StructFieldType.kUint8, false, true, 8, 0xff), - Arguments.of("uint16 a", 2, StructFieldType.kUint16, false, true, 16, 0xffff), - Arguments.of("uint32 a", 4, StructFieldType.kUint32, false, true, 32, 0xffffffffL), - Arguments.of("uint64 a", 8, StructFieldType.kUint64, false, true, 64, -1), - Arguments.of("float a", 4, StructFieldType.kFloat, false, false, 32, 0xffffffffL), - Arguments.of("float32 a", 4, StructFieldType.kFloat, false, false, 32, 0xffffffffL), - Arguments.of("double a", 8, StructFieldType.kDouble, false, false, 64, -1), - Arguments.of("float64 a", 8, StructFieldType.kDouble, false, false, 64, -1), - Arguments.of("foo a", 0, StructFieldType.kStruct, false, false, 0, 0)); + Arguments.of("bool a", 1, StructFieldType.kBool, false, false, 8, 0xff, 0, 0), + Arguments.of("char a", 1, StructFieldType.kChar, false, false, 8, 0xff, 0, 0), + Arguments.of("int8 a", 1, StructFieldType.kInt8, true, false, 8, 0xff, -128, 127), + Arguments.of("int16 a", 2, StructFieldType.kInt16, true, false, 16, 0xffff, -32768, 32767), + Arguments.of( + "int32 a", + 4, + StructFieldType.kInt32, + true, + false, + 32, + 0xffffffffL, + -2147483648, + 2147483647), + Arguments.of( + "int64 a", + 8, + StructFieldType.kInt64, + true, + false, + 64, + -1, + -9223372036854775808L, + 9223372036854775807L), + Arguments.of("uint8 a", 1, StructFieldType.kUint8, false, true, 8, 0xff, 0, 255), + Arguments.of("uint16 a", 2, StructFieldType.kUint16, false, true, 16, 0xffff, 0, 65535), + Arguments.of( + "uint32 a", 4, StructFieldType.kUint32, false, true, 32, 0xffffffffL, 0, 4294967295L), + Arguments.of("uint64 a", 8, StructFieldType.kUint64, false, true, 64, -1, 0, 0), + Arguments.of("float a", 4, StructFieldType.kFloat, false, false, 32, 0xffffffffL, 0, 0), + Arguments.of("float32 a", 4, StructFieldType.kFloat, false, false, 32, 0xffffffffL, 0, 0), + Arguments.of("double a", 8, StructFieldType.kDouble, false, false, 64, -1, 0, 0), + Arguments.of("float64 a", 8, StructFieldType.kDouble, false, false, 64, -1, 0, 0), + Arguments.of("foo a", 0, StructFieldType.kStruct, false, false, 0, 0, 0, 0)); } @ParameterizedTest @@ -409,6 +428,40 @@ void testStandardArray( } } + @ParameterizedTest + @MethodSource("provideSimpleTestParams") + void testIntRoundTrip( + String schema, + int size, + StructFieldType type, + boolean isInt, + boolean isUint, + int bitWidth, + long bitMask, + long minVal, + long maxVal) { + if (type == StructFieldType.kStruct) { + return; + } + var desc = assertDoesNotThrow(() -> db.add("test", schema)); + assertTrue(desc.isValid()); + var dynamic = DynamicStruct.allocate(desc); + var field = desc.findFieldByName("a"); + assertNotNull(field); + if ((isInt || isUint) && type != StructFieldType.kUint64) { + // Java can't represent uint64 max + dynamic.setIntField(field, minVal); + assertEquals(minVal, dynamic.getIntField(field)); + dynamic.setIntField(field, maxVal); + assertEquals(maxVal, dynamic.getIntField(field)); + } else if (type == StructFieldType.kBool) { + dynamic.setBoolField(field, false); + assertFalse(dynamic.getBoolField(field)); + dynamic.setBoolField(field, true); + assertTrue(dynamic.getBoolField(field)); + } + } + @Test void testStringAllZeros() { var desc = assertDoesNotThrow(() -> db.add("test", "char a[32]")); diff --git a/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp b/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp index 3344ce4ac79..d7929e17443 100644 --- a/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp +++ b/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp @@ -7,7 +7,9 @@ #include #include +#include #include +#include #include @@ -473,6 +475,8 @@ struct SimpleTestParam { bool isUint; unsigned int bitWidth; uint64_t bitMask; + uint64_t minVal; + uint64_t maxVal; }; std::ostream& operator<<(std::ostream& os, const SimpleTestParam& param) { @@ -532,22 +536,94 @@ TEST_P(DynamicSimpleStructTest, Array) { } } +static int64_t SignExtend(uint64_t value, size_t size) { + switch (size) { + case 1: + return static_cast(value); + case 2: + return static_cast(value); + case 4: + return static_cast(value); + default: + return value; + } +} + +TEST_P(DynamicSimpleStructTest, IntRoundTrip) { + if (GetParam().type == StructFieldType::kStruct) { + return; + } + auto desc = db.Add("test", GetParam().schema, &err); + ASSERT_TRUE(desc); + ASSERT_TRUE(desc->IsValid()); + std::vector dest(desc->GetSize()); + auto field = desc->FindFieldByName("a"); + ASSERT_TRUE(field); + wpi::MutableDynamicStruct dynamic(desc, dest); + if (GetParam().isInt) { + { + int64_t value = SignExtend(GetParam().minVal, field->GetSize()); + dynamic.SetIntField(field, value); + EXPECT_EQ(dynamic.GetIntField(field), value); + } + { + int64_t value = SignExtend(GetParam().maxVal, field->GetSize()); + dynamic.SetIntField(field, value); + EXPECT_EQ(dynamic.GetIntField(field), value); + } + } else if (GetParam().isUint) { + { + uint64_t value = GetParam().minVal; + dynamic.SetUintField(field, value); + EXPECT_EQ(dynamic.GetUintField(field), value); + } + { + uint64_t value = GetParam().maxVal; + dynamic.SetUintField(field, value); + EXPECT_EQ(dynamic.GetUintField(field), value); + } + } else if (GetParam().type == StructFieldType::kBool) { + dynamic.SetBoolField(field, false); + EXPECT_FALSE(dynamic.GetBoolField(field)); + dynamic.SetBoolField(field, true); + EXPECT_TRUE(dynamic.GetBoolField(field)); + } +} + static SimpleTestParam simpleTests[] = { - {"bool a", 1, StructFieldType::kBool, false, false, 8, UINT8_MAX}, - {"char a", 1, StructFieldType::kChar, false, false, 8, UINT8_MAX}, - {"int8 a", 1, StructFieldType::kInt8, true, false, 8, UINT8_MAX}, - {"int16 a", 2, StructFieldType::kInt16, true, false, 16, UINT16_MAX}, - {"int32 a", 4, StructFieldType::kInt32, true, false, 32, UINT32_MAX}, - {"int64 a", 8, StructFieldType::kInt64, true, false, 64, UINT64_MAX}, - {"uint8 a", 1, StructFieldType::kUint8, false, true, 8, UINT8_MAX}, - {"uint16 a", 2, StructFieldType::kUint16, false, true, 16, UINT16_MAX}, - {"uint32 a", 4, StructFieldType::kUint32, false, true, 32, UINT32_MAX}, - {"uint64 a", 8, StructFieldType::kUint64, false, true, 64, UINT64_MAX}, - {"float a", 4, StructFieldType::kFloat, false, false, 32, UINT32_MAX}, - {"float32 a", 4, StructFieldType::kFloat, false, false, 32, UINT32_MAX}, - {"double a", 8, StructFieldType::kDouble, false, false, 64, UINT64_MAX}, - {"float64 a", 8, StructFieldType::kDouble, false, false, 64, UINT64_MAX}, - {"foo a", 0, StructFieldType::kStruct, false, false, 0, 0}, + {"bool a", 1, StructFieldType::kBool, false, false, 8, UINT8_MAX, 0, 0}, + {"char a", 1, StructFieldType::kChar, false, false, 8, UINT8_MAX, 0, 0}, + {"int8 a", 1, StructFieldType::kInt8, true, false, 8, UINT8_MAX, + static_cast(std::numeric_limits::min()), + std::numeric_limits::max()}, + {"int16 a", 2, StructFieldType::kInt16, true, false, 16, UINT16_MAX, + static_cast(std::numeric_limits::min()), + std::numeric_limits::max()}, + {"int32 a", 4, StructFieldType::kInt32, true, false, 32, UINT32_MAX, + static_cast(std::numeric_limits::min()), + std::numeric_limits::max()}, + {"int64 a", 8, StructFieldType::kInt64, true, false, 64, UINT64_MAX, + static_cast(std::numeric_limits::min()), + std::numeric_limits::max()}, + {"uint8 a", 1, StructFieldType::kUint8, false, true, 8, UINT8_MAX, + std::numeric_limits::min(), std::numeric_limits::max()}, + {"uint16 a", 2, StructFieldType::kUint16, false, true, 16, UINT16_MAX, + std::numeric_limits::min(), + std::numeric_limits::max()}, + {"uint32 a", 4, StructFieldType::kUint32, false, true, 32, UINT32_MAX, + std::numeric_limits::min(), + std::numeric_limits::max()}, + {"uint64 a", 8, StructFieldType::kUint64, false, true, 64, UINT64_MAX, + std::numeric_limits::min(), + std::numeric_limits::max()}, + {"float a", 4, StructFieldType::kFloat, false, false, 32, UINT32_MAX, 0, 0}, + {"float32 a", 4, StructFieldType::kFloat, false, false, 32, UINT32_MAX, 0, + 0}, + {"double a", 8, StructFieldType::kDouble, false, false, 64, UINT64_MAX, 0, + 0}, + {"float64 a", 8, StructFieldType::kDouble, false, false, 64, UINT64_MAX, 0, + 0}, + {"foo a", 0, StructFieldType::kStruct, false, false, 0, 0, 0, 0}, }; INSTANTIATE_TEST_SUITE_P(DynamicSimpleStructTests, DynamicSimpleStructTest,