Skip to content

Commit

Permalink
RSDK-8828 Remove integer type from ProtoValue (#292)
Browse files Browse the repository at this point in the history 
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
  • Loading branch information
@lia-viam @dependabot
lia-viam and dependabot[bot] authored Sep 24, 2024
1 parent 32b31fa commit eefcbd6
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 82 deletions.
31 changes: 17 additions & 14 deletions src/viam/sdk/common/proto_value.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,28 @@ namespace sdk {
using google::protobuf::Struct;
using google::protobuf::Value;

ProtoValue::ProtoValue() noexcept : ProtoValue(nullptr) {}
ProtoValue::ProtoValue() noexcept : ProtoValue(nullptr, nullptr) {}
ProtoValue::ProtoValue(std::nullptr_t) noexcept : ProtoValue() {}
ProtoValue::ProtoValue(bool b) noexcept : ProtoValue(b, nullptr) {}
ProtoValue::ProtoValue(int i) noexcept : ProtoValue(static_cast<double>(i)) {}
ProtoValue::ProtoValue(double d) noexcept : ProtoValue(d, nullptr) {}
ProtoValue::ProtoValue(std::string s) noexcept : ProtoValue(std::move(s), nullptr) {}

template <typename Val, typename>
ProtoValue::ProtoValue(std::vector<Val> v) noexcept(
std::is_nothrow_move_constructible<std::vector<Val>>{})
: ProtoValue(std::move(v), 0) {}

template <typename Val, typename>
ProtoValue::ProtoValue(std::unordered_map<std::string, Val> s) noexcept(
std::is_nothrow_move_constructible<std::unordered_map<std::string, Val>>{})
: ProtoValue(std::move(s), 0) {}

template <typename T>
ProtoValue::ProtoValue(T t) noexcept(std::is_nothrow_move_constructible<T>{})
ProtoValue::ProtoValue(T t, std::nullptr_t) noexcept(std::is_nothrow_move_constructible<T>{})
: vtable_{model<T>::vtable_}, self_{std::move(t)} {}

// -- explicit instantiations of by-value constructors -- //
template ProtoValue::ProtoValue(std::nullptr_t) noexcept;
template ProtoValue::ProtoValue(bool) noexcept;
template ProtoValue::ProtoValue(int) noexcept;
template ProtoValue::ProtoValue(double) noexcept;
template ProtoValue::ProtoValue(std::string) noexcept(
std::is_nothrow_move_constructible<std::string>{});
template ProtoValue::ProtoValue(ProtoList) noexcept(
std::is_nothrow_move_constructible<ProtoList>{});
template ProtoValue::ProtoValue(ProtoStruct m) noexcept(
Expand Down Expand Up @@ -117,11 +126,9 @@ std::enable_if_t<std::is_scalar<T>{}, T> ProtoValue::get_unchecked() const {
}

template bool& ProtoValue::get_unchecked<bool>();
template int& ProtoValue::get_unchecked<int>();
template double& ProtoValue::get_unchecked<double>();

template bool ProtoValue::get_unchecked<bool>() const;
template int ProtoValue::get_unchecked<int>() const;
template double ProtoValue::get_unchecked<double>() const;

template <typename T>
Expand Down Expand Up @@ -255,10 +262,6 @@ void to_proto(bool b, Value* v) {
v->set_bool_value(b);
}

void to_proto(int i, Value* v) {
v->set_number_value(i);
}

void to_proto(double d, Value* v) {
v->set_number_value(d);
}
Expand Down
64 changes: 35 additions & 29 deletions src/viam/sdk/common/proto_value.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,42 @@ struct all_moves_noexcept
class ProtoValue {
public:
/// @brief Type discriminator constants for possible values stored in a ProtoValue.
enum Kind {
k_null = 0,
k_bool = 1,
k_double = 2,
k_string = 3,
k_list = 4,
k_struct = 5,
k_int = 6
};
enum Kind { k_null = 0, k_bool = 1, k_double = 2, k_string = 3, k_list = 4, k_struct = 5 };

/// @brief Construct a null object.
ProtoValue() noexcept;

/// @brief Construct a nonempty object.
template <typename T>
ProtoValue(T t) noexcept(std::is_nothrow_move_constructible<T>{});
/// @name Value constructors.
/// @brief Constructors which initialize a ProtoValue holding its argument.
/// @{

ProtoValue(std::nullptr_t) noexcept;

ProtoValue(bool b) noexcept;

/// @brief Construct a double object upcast from constructor argument.
ProtoValue(int i) noexcept;

ProtoValue(double d) noexcept;

ProtoValue(std::string s) noexcept;

/// @brief Deduction helper constructor for string from string literal
ProtoValue(const char* str);

/// @brief Construct from a ProtoList.
template <typename Val = ProtoValue,
typename = std::enable_if_t<std::is_same<Val, ProtoValue>{}>>
ProtoValue(std::vector<Val>) noexcept(std::is_nothrow_move_constructible<std::vector<Val>>{});

/// @brief Construct from a ProtoStruct.
template <typename Val = ProtoValue,
typename = std::enable_if_t<std::is_same<Val, ProtoValue>{}>>
ProtoValue(std::unordered_map<std::string, Val>) noexcept(
std::is_nothrow_move_constructible<std::unordered_map<std::string, Val>>{});

/// @}

/// @brief Move construct this from other, leaving other in its unspecified-but-valid moved from
/// state.
ProtoValue(ProtoValue&& other) noexcept(proto_value_details::all_moves_noexcept{});
Expand Down Expand Up @@ -125,12 +141,12 @@ class ProtoValue {
T const* get() const;

/// @brief Return a reference to the underlying T, without checking.
/// @tparam T a bool, int, or double
/// @tparam T a bool or double
template <typename T>
std::enable_if_t<std::is_scalar<T>{}, T&> get_unchecked();

/// @brief Return the underlying T by value, without checking.
/// @tparam T a bool, int, or double.
/// @tparam T a bool or double.
template <typename T>
std::enable_if_t<std::is_scalar<T>{}, T> get_unchecked() const;

Expand Down Expand Up @@ -204,7 +220,6 @@ class ProtoValue {
using BufType = std::aligned_union_t<0,
std::nullptr_t,
bool,
int,
double,
std::string,
std::vector<void*>,
Expand Down Expand Up @@ -257,6 +272,11 @@ class ProtoValue {

ProtoValue(const google::protobuf::Value* value);

// Helper template for the explicit versions above.
// Includes nullptr_t as a tag type so we can let the other constructors delegate.
template <typename T>
ProtoValue(T t, std::nullptr_t) noexcept(std::is_nothrow_move_constructible<T>{});

vtable vtable_;
storage self_;
};
Expand All @@ -275,24 +295,16 @@ using ProtoList = std::vector<ProtoValue>;
using ProtoStruct = std::unordered_map<std::string, ProtoValue>;

// -- Template specialization declarations of by-value constructors -- //
extern template ProtoValue::ProtoValue(std::nullptr_t) noexcept;
extern template ProtoValue::ProtoValue(bool) noexcept;
extern template ProtoValue::ProtoValue(int) noexcept;
extern template ProtoValue::ProtoValue(double) noexcept;
extern template ProtoValue::ProtoValue(std::string) noexcept(
std::is_nothrow_move_constructible<std::string>{});
extern template ProtoValue::ProtoValue(ProtoList) noexcept(
std::is_nothrow_move_constructible<ProtoList>{});
extern template ProtoValue::ProtoValue(ProtoStruct m) noexcept(
std::is_nothrow_move_constructible<ProtoStruct>{});

// -- Template specialization declarations of get_unchecked: POD types -- //
extern template bool& ProtoValue::get_unchecked<bool>();
extern template int& ProtoValue::get_unchecked<int>();
extern template double& ProtoValue::get_unchecked<double>();

extern template bool ProtoValue::get_unchecked<bool>() const;
extern template int ProtoValue::get_unchecked<int>() const;
extern template double ProtoValue::get_unchecked<double>() const;

// -- Template specialization declarations of get_unchecked: string and recursive types -- //
Expand All @@ -310,7 +322,6 @@ extern template ProtoStruct&& ProtoValue::get_unchecked<ProtoStruct>() &&;

void to_proto(std::nullptr_t, google::protobuf::Value* v);
void to_proto(bool b, google::protobuf::Value* v);
void to_proto(int i, google::protobuf::Value* v);
void to_proto(double d, google::protobuf::Value* v);
void to_proto(std::string s, google::protobuf::Value* v);
void to_proto(const ProtoList& vec, google::protobuf::Value* v);
Expand Down Expand Up @@ -371,11 +382,6 @@ struct kind<bool> {
using type = KindConstant<ProtoValue::Kind::k_bool>;
};

template <>
struct kind<int> {
using type = KindConstant<ProtoValue::Kind::k_int>;
};

template <>
struct kind<double> {
using type = KindConstant<ProtoValue::Kind::k_double>;
Expand Down
6 changes: 0 additions & 6 deletions src/viam/sdk/common/proto_value_visit.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ auto visit(Visitor&& visitor, ProtoValue& value)
switch (value.kind()) {
case ProtoValue::Kind::k_bool:
return std::forward<Visitor>(visitor)(value.get_unchecked<bool>());
case ProtoValue::Kind::k_int:
return std::forward<Visitor>(visitor)(value.get_unchecked<int>());
case ProtoValue::Kind::k_double:
return std::forward<Visitor>(visitor)(value.get_unchecked<double>());
case ProtoValue::Kind::k_string:
Expand All @@ -47,8 +45,6 @@ auto visit(Visitor&& visitor, const ProtoValue& value)
switch (value.kind()) {
case ProtoValue::Kind::k_bool:
return std::forward<Visitor>(visitor)(value.get_unchecked<bool>());
case ProtoValue::Kind::k_int:
return std::forward<Visitor>(visitor)(value.get_unchecked<int>());
case ProtoValue::Kind::k_double:
return std::forward<Visitor>(visitor)(value.get_unchecked<double>());
case ProtoValue::Kind::k_string:
Expand All @@ -70,8 +66,6 @@ auto visit(Visitor&& visitor, ProtoValue&& value)
switch (value.kind()) {
case ProtoValue::Kind::k_bool:
return std::forward<Visitor>(visitor)(std::move(value.get_unchecked<bool>()));
case ProtoValue::Kind::k_int:
return std::forward<Visitor>(visitor)(std::move(value.get_unchecked<int>()));
case ProtoValue::Kind::k_double:
return std::forward<Visitor>(visitor)(std::move(value.get_unchecked<double>()));
case ProtoValue::Kind::k_string:
Expand Down
31 changes: 3 additions & 28 deletions src/viam/sdk/tests/test_proto_value.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,32 +38,9 @@ BOOST_AUTO_TEST_CASE(test_object_equality) {

// heterogeneous arithmetic types do not "inuitively" compare equal
BOOST_CHECK(!(ProtoValue(false) == ProtoValue(0)));
BOOST_CHECK(!(ProtoValue(0) == ProtoValue(0.0)));

// roundtrip integer conversion is not idempotent because the integer gets coerced to a double
{
ProtoValue i5(5);
auto int_roundtrip = ProtoValue::from_proto(to_proto(i5));
BOOST_CHECK(i5.kind() == ProtoValue::Kind::k_int);
BOOST_CHECK(i5.is_a<int>());

BOOST_CHECK(int_roundtrip.kind() == ProtoValue::Kind::k_double);
BOOST_CHECK(int_roundtrip.is_a<double>());

BOOST_CHECK(!(i5 == int_roundtrip));
BOOST_CHECK(int_roundtrip == ProtoValue(5.0));

ProtoValue i5_copy(i5);
BOOST_CHECK(i5_copy == i5);

ProtoValue i5_move(std::move(i5));
BOOST_CHECK(i5_copy == i5_move);
BOOST_CHECK(i5.is_a<int>());
}

auto test_cases = std::make_tuple(
std::make_pair(true, false),
/* integer not included, see above */
std::make_pair(6.0, 7.5),
std::make_pair(std::string("string"), std::string("different")),
std::make_pair(ProtoList({ProtoValue{"asdf"}}),
Expand All @@ -82,7 +59,6 @@ BOOST_AUTO_TEST_CASE(test_object_equality) {
ProtoValue v1(test_pair.first);
BOOST_CHECK(v1.kind() == kind);
BOOST_CHECK(v1.is_a<test_type>());
BOOST_CHECK(!v1.get<int>());

{
const test_type* ptr = v1.get<test_type>();
Expand Down Expand Up @@ -182,7 +158,9 @@ BOOST_AUTO_TEST_CASE(test_unchecked_access) {
ProtoStruct{{"int", 5}, {"double", 6.0}}));

tuple_for_each(std::tuple_cat(scalar_tests, nonscalar_tests), [](auto test_pair) {
using test_type = typename decltype(test_pair)::first_type;
using first_type = typename decltype(test_pair)::first_type;
using test_type = std::conditional_t<std::is_same<first_type, int>{}, double, first_type>;

const test_type first = test_pair.first;
const test_type second = test_pair.second;

Expand Down Expand Up @@ -249,9 +227,6 @@ BOOST_AUTO_TEST_CASE(test_nested_objects) {
void set_proto_value(Value& val, bool b) {
val.set_bool_value(b);
}
void set_proto_value(Value& val, int i) {
val.set_number_value(i);
}
void set_proto_value(Value& val, double d) {
val.set_number_value(d);
}
Expand Down
5 changes: 0 additions & 5 deletions src/viam/sdk/tests/test_proto_value_visit.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,6 @@ struct rvalue_visitor {
return kind == ProtoValue::Kind::k_bool;
}

bool operator()(int&&) && {
return kind == ProtoValue::Kind::k_int;
}

bool operator()(double&&) && {
return kind == ProtoValue::Kind::k_double;
}
Expand Down Expand Up @@ -133,7 +129,6 @@ BOOST_AUTO_TEST_CASE(test_visitor) {
auto test_cases = std::make_tuple(
std::make_pair(ProtoValue::Kind::k_null, nullptr),
std::make_pair(ProtoValue::Kind::k_bool, true),
std::make_pair(ProtoValue::Kind::k_int, 5),
std::make_pair(ProtoValue::Kind::k_double, 12.345),
std::make_pair(ProtoValue::Kind::k_string, "meow"),
std::make_pair(ProtoValue::Kind::k_list, ProtoList({{ProtoValue(1), ProtoValue("woof")}})),
Expand Down

0 comments on commit eefcbd6

Please sign in to comment.