address review comments

Author: sancar

hazelcast/include/hazelcast/client/decimal.h

@@ -49,6 +49,19 @@ struct HAZELCAST_API decimal
 bool HAZELCAST_API
 operator==(const decimal& lhs, const decimal& rhs);
 
+bool HAZELCAST_API
+operator<(const decimal& lhs, const decimal& rhs);
+} // namespace client
+} // namespace hazelcast
+namespace std {
+template<>
+struct HAZELCAST_API hash<hazelcast::client::decimal>
+{
+    std::size_t operator()(const hazelcast::client::decimal& f) const;
+};
+} // namespace std
+namespace hazelcast {
+namespace client {
 namespace pimpl {
 
 /**
@@ -79,7 +92,8 @@ from_bytes(std::vector<int8_t> v);
  * if i is a negative number, we take the two's complement on resulting vector,
  * to get negative representation of the number.
  * We also add one extra byte to the end of the vector to preserve the sign if
- * necessary.
+ * sign of the integer is not same as the most significant byte's sign.
+ * Otherwise we don't add it to have minimum size vector to represent the value.
  * @param i the number to convert to bytes
  * @return the vector of int8_t representing the number
  */

hazelcast/src/hazelcast/client/client_impl.cpp

@@ -45,7 +45,7 @@
 #include "hazelcast/logger.h"
 #include "hazelcast/client/member_selectors.h"
 #include "hazelcast/client/client_properties.h"
-
+#include "hazelcast/client/decimal.h"
 #ifndef HAZELCAST_VERSION
 #define HAZELCAST_VERSION "NOT_FOUND"
 #endif
@@ -591,6 +591,79 @@ operator<<(std::ostream& stream, const address& address)
     return stream << address.to_string();
 }
 
+bool
+operator==(const decimal& lhs, const decimal& rhs)
+{
+    return lhs.unscaled == rhs.unscaled && lhs.scale == rhs.scale;
+}
+
+bool
+operator<(const decimal& lhs, const decimal& rhs)
+{
+    if (lhs.scale != rhs.scale) {
+        return lhs.scale < rhs.scale;
+    }
+    return lhs.unscaled < rhs.unscaled;
+}
+
+namespace pimpl {
+
+void
+twos_complement(std::vector<int8_t>& a)
+{
+    // twos complement is calculated via flipping the bits and adding 1
+    // flip the bits
+    for (auto& item : a) {
+        item = ~item;
+    }
+    // add 1
+    int8_t carry = 1;
+    for (int i = a.size() - 1; i >= 0; i--) {
+        a[i] = a[i] + carry;
+        if (a[i] == 0) {
+            carry = 1;
+        } else {
+            break;
+        }
+    }
+}
+
+boost::multiprecision::cpp_int
+from_bytes(std::vector<int8_t> v)
+{
+    boost::multiprecision::cpp_int i;
+    bool is_negative = v[0] < 0;
+    if (is_negative) {
+        twos_complement(v);
+    }
+    import_bits(i, v.begin(), v.end(), 8);
+    if (is_negative) {
+        return -i;
+    }
+    return i;
+}
+
+std::vector<int8_t>
+to_bytes(const boost::multiprecision::cpp_int& i)
+{
+    std::vector<int8_t> v;
+    export_bits(i, std::back_inserter(v), 8);
+    if (i < 0) {
+        twos_complement(v);
+        if (v[0] > 0) {
+            // add -1 as the most significant to have a negative sign bit
+            v.insert(v.begin(), -1);
+        }
+    } else {
+        // add 0 as the most significant byte to have a positive sign bit
+        if (v[0] < 0) {
+            v.insert(v.begin(), 0);
+        }
+    }
+    return v;
+}
+} // namespace pimpl
+
 namespace serialization {
 int32_t
 hz_serializer<address>::get_factory_id()
@@ -1152,4 +1225,14 @@ hash<hazelcast::client::address>::operator()(
     boost::hash_combine(seed, address.type_);
     return seed;
 }
+
+std::size_t
+hash<hazelcast::client::decimal>::operator()(
+  const hazelcast::client::decimal& dec) const
+{
+    std::size_t seed = 0;
+    boost::hash_combine(seed, dec.unscaled);
+    boost::hash_combine(seed, dec.scale);
+    return seed;
+};
 } // namespace std

hazelcast/src/hazelcast/client/decimal.cpp

@@ -81,8 +81,8 @@ class DecimalTest : public ::testing::Test
 {};
 
 void
-assertEqual(const std::string& expected_string,
-            const std::vector<int8_t>& expected_vector)
+assert_equal(const std::string& expected_string,
+             const std::vector<int8_t>& expected_vector)
 {
     using boost::multiprecision::cpp_int;
     cpp_int expected_int(expected_string);
@@ -95,7 +95,7 @@ assertEqual(const std::string& expected_string,
 
 TEST_F(DecimalTest, positive_test)
 {
-    assertEqual(
+    assert_equal(
       "236095134049630962491764987683473058401811134068823290126231516129",
       {
         2,  61, -22, 92, -44, -54, -45, -9,  -17, -4, -66, -5,  -12, 19,
@@ -105,21 +105,21 @@ TEST_F(DecimalTest, positive_test)
 
 TEST_F(DecimalTest, negative_test)
 {
-    assertEqual("-158058224523514071900098807210097354699988293366",
-                {
-                  -28, 80,  108, -112, -19, -44, 84,  -98,  96,  106,
-                  53,  -88, 77,  -45,  89,  119, 109, -109, -87, 10,
-                });
+    assert_equal("-158058224523514071900098807210097354699988293366",
+                 {
+                   -28, 80,  108, -112, -19, -44, 84,  -98,  96,  106,
+                   53,  -88, 77,  -45,  89,  119, 109, -109, -87, 10,
+                 });
 }
 
 TEST_F(DecimalTest, preserve_positive_sign_test)
 {
-    assertEqual("53220513728803604", { 0, -67, 19, -58, 119, -111, -77, 20 });
+    assert_equal("53220513728803604", { 0, -67, 19, -58, 119, -111, -77, 20 });
 }
 
 TEST_F(DecimalTest, preserve_negative_sign_test)
 {
-    assertEqual(
+    assert_equal(
       "-78097809300018214368298043748751294327036591272091714272720014418",
       {
         -1,  66, 39,  -99,  44,  53, -23, 60,  125, 105, 65,   -21, 104, -36,
@@ -129,21 +129,21 @@ TEST_F(DecimalTest, preserve_negative_sign_test)
 
 TEST_F(DecimalTest, carry_bit_test)
 {
-    assertEqual("-4172290065390264938962604145655817690459633380799476516330728"
-                "71499276353298132342018230923743606150479511296",
-                {
-                  -46,  -123, 61, 41,   -1,  115,  -54,  91,   -48,
-                  79,   55,   25, 41,   -90, 14,   109,  -115, 68,
-                  -122, 46,   70, 90,   47,  -103, -21,  -39,  126,
-                  -45,  37,   58, 60,   -76, -44,  91,   97,   52,
-                  31,   -38,  23, -111, 18,  -112, -109, -127, 0,
-                });
+    assert_equal(
+      "-4172290065390264938962604145655817690459633380799476516330728"
+      "71499276353298132342018230923743606150479511296",
+      {
+        -46, -123, 61,  41,   -1,   115,  -54,  91,   -48, 79, 55, 25,
+        41,  -90,  14,  109,  -115, 68,   -122, 46,   70,  90, 47, -103,
+        -21, -39,  126, -45,  37,   58,   60,   -76,  -44, 91, 97, 52,
+        31,  -38,  23,  -111, 18,   -112, -109, -127, 0,
+      });
 }
 
 TEST_F(DecimalTest, cascading_carry_bit_test)
 {
-    assertEqual("-1234506895138773532672",
-                { -67, 19, -58, 119, -111, -77, 0, 0, 0 });
+    assert_equal("-1234506895138773532672",
+                 { -67, 19, -58, 119, -111, -77, 0, 0, 0 });
 }
 
 class AddressHelperTest : public ClientTest

hazelcast/test/src/HazelcastTests2.cpp

@@ -25,6 +25,7 @@
 #endif
 
 #include <hazelcast/client/serialization/serialization.h>
+#include <hazelcast/client/decimal.h>
 
 #if defined(WIN32) || defined(_WIN32) || defined(WIN64) || defined(_WIN64)
 #pragma warning(push)
@@ -494,6 +495,9 @@ check_schema_field(const schema& schema,
 
 TEST_F(CompactSerializationTest, test_field_order_fixed_size)
 {
+    std::unordered_map<hazelcast::client::decimal, hazelcast::client::decimal> m(4);
+    m.insert(std::make_pair(hazelcast::client::decimal{1, 2},
+                            hazelcast::client::decimal{3, 4}));
     schema_writer schema_writer("typeName");
     auto writer = serialization::pimpl::create_compact_writer(&schema_writer);
     serialization::hz_serializer<employee_dto>::write(employee_dto{}, writer);