Skip to content

Commit

Permalink
[wpiutil] DynamicStruct: Fix decoding of signed integers (#7350)
Browse files Browse the repository at this point in the history 
Add tests for both C++ and Java.
  • Loading branch information
@rzblue
rzblue authored Nov 6, 2024
1 parent 3113627 commit 83fa422
Show file tree
Hide file tree
Showing 3 changed files with 170 additions and 31 deletions.
12 changes: 11 additions & 1 deletion wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int8_t>(raw);
case 2:
return static_cast<int16_t>(raw);
case 4:
return static_cast<int32_t>(raw);
default:
return raw;
}
}

/**
Expand Down
83 changes: 68 additions & 15 deletions wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,21 +328,40 @@ void testDuplicateFieldName() {

private static Stream<Arguments> 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
Expand Down Expand Up @@ -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]"));
Expand Down
106 changes: 91 additions & 15 deletions wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
#include <stdint.h>

#include <cstring>
#include <limits>
#include <string>
#include <vector>

#include <gtest/gtest.h>

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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<int8_t>(value);
case 2:
return static_cast<int16_t>(value);
case 4:
return static_cast<int32_t>(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<uint8_t> 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<uint64_t>(std::numeric_limits<int8_t>::min()),
std::numeric_limits<int8_t>::max()},
{"int16 a", 2, StructFieldType::kInt16, true, false, 16, UINT16_MAX,
static_cast<uint64_t>(std::numeric_limits<int16_t>::min()),
std::numeric_limits<int16_t>::max()},
{"int32 a", 4, StructFieldType::kInt32, true, false, 32, UINT32_MAX,
static_cast<uint64_t>(std::numeric_limits<int32_t>::min()),
std::numeric_limits<int32_t>::max()},
{"int64 a", 8, StructFieldType::kInt64, true, false, 64, UINT64_MAX,
static_cast<uint64_t>(std::numeric_limits<int64_t>::min()),
std::numeric_limits<int64_t>::max()},
{"uint8 a", 1, StructFieldType::kUint8, false, true, 8, UINT8_MAX,
std::numeric_limits<uint8_t>::min(), std::numeric_limits<uint8_t>::max()},
{"uint16 a", 2, StructFieldType::kUint16, false, true, 16, UINT16_MAX,
std::numeric_limits<uint16_t>::min(),
std::numeric_limits<uint16_t>::max()},
{"uint32 a", 4, StructFieldType::kUint32, false, true, 32, UINT32_MAX,
std::numeric_limits<uint32_t>::min(),
std::numeric_limits<uint32_t>::max()},
{"uint64 a", 8, StructFieldType::kUint64, false, true, 64, UINT64_MAX,
std::numeric_limits<uint64_t>::min(),
std::numeric_limits<uint64_t>::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,
Expand Down

0 comments on commit 83fa422

Please sign in to comment.