isapego commented on code in PR #1104:
URL: https://github.com/apache/ignite-3/pull/1104#discussion_r984346220
##########
modules/platforms/cpp/schema/BinaryTupleParser.cpp:
##########
@@ -18,22 +18,61 @@
#include "BinaryTupleParser.h"
#include "common/Platform.h"
+#include "common/Bytes.h"
+
#include <cassert>
#include <cstring>
#include <stdexcept>
+namespace ignite {
+
namespace {
template <typename T>
-T as(ignite::BytesView bytes) noexcept {
- T value;
- std::memcpy(&value, bytes.data(), sizeof(T));
- return value;
+T loadBytesAs(BytesView bytes, std::size_t offset = 0) noexcept {
+ return bytes::load<Endian::LITTLE, T>(bytes.data() + offset);
}
-} // namespace
+Date loadBytesAsDate(ignite::BytesView bytes) {
+ std::int32_t date = loadBytesAs<std::uint16_t>(bytes);
+ date |= std::int32_t(loadBytesAs<std::int8_t>(bytes, 2)) << 16;
-namespace ignite {
+ std::int32_t day = date & 31;
+ std::int32_t month = (date >> 5) & 15;
+ std::int32_t year = (date >> 9); // Sign matters.
+
+ return Date(year, month, day);
+}
+
+Time loadBytesAsTime(ignite::BytesView bytes) {
+ uint64_t time = loadBytesAs<std::uint32_t>(bytes);
+
+ int nano;
Review Comment:
Why is it `int`? Can it be negative? And why not to specify size which is
enough to fit the max value (1'000'000'000 I believe)?
##########
modules/platforms/cpp/schema/BinaryTupleParser.cpp:
##########
@@ -18,22 +18,61 @@
#include "BinaryTupleParser.h"
#include "common/Platform.h"
+#include "common/Bytes.h"
+
#include <cassert>
#include <cstring>
#include <stdexcept>
+namespace ignite {
+
namespace {
template <typename T>
-T as(ignite::BytesView bytes) noexcept {
- T value;
- std::memcpy(&value, bytes.data(), sizeof(T));
- return value;
+T loadBytesAs(BytesView bytes, std::size_t offset = 0) noexcept {
+ return bytes::load<Endian::LITTLE, T>(bytes.data() + offset);
}
-} // namespace
+Date loadBytesAsDate(ignite::BytesView bytes) {
+ std::int32_t date = loadBytesAs<std::uint16_t>(bytes);
+ date |= std::int32_t(loadBytesAs<std::int8_t>(bytes, 2)) << 16;
-namespace ignite {
+ std::int32_t day = date & 31;
+ std::int32_t month = (date >> 5) & 15;
+ std::int32_t year = (date >> 9); // Sign matters.
+
+ return Date(year, month, day);
+}
+
+Time loadBytesAsTime(ignite::BytesView bytes) {
+ uint64_t time = loadBytesAs<std::uint32_t>(bytes);
+
+ int nano;
+ switch (bytes.size()) {
+ case 4:
+ nano = ((int)time & ((1 << 10) - 1)) * 1000 * 1000;
+ time >>= 10;
+ break;
+ case 5:
+ time |= std::uint64_t(loadBytesAs<std::uint8_t>(bytes, 4)) << 32;
+ nano = ((int)time & ((1 << 20) - 1)) * 1000;
+ time >>= 20;
+ break;
+ case 6:
+ time |= std::uint64_t(loadBytesAs<std::uint16_t>(bytes, 4)) << 32;
+ nano = ((int)time & ((1 << 30) - 1));
+ time >>= 30;
+ break;
+ }
+
+ int second = ((int)time) & 63;
+ int minute = ((int)time >> 6) & 63;
+ int hour = ((int)time >> 12) & 31;
Review Comment:
Using bit shift operation on `int` value implies it have a certain size in
bits. And while it's probably true for the most modern systems, why not use
sized type here just to be safe?
##########
modules/platforms/cpp/schema/BinaryTupleBuilder.cpp:
##########
@@ -104,45 +106,45 @@ void BinaryTupleBuilder::putBytes(BytesView bytes) {
void BinaryTupleBuilder::putInt8(BytesView bytes) {
SizeT size = sizeOfInt8(BinaryTupleParser::getInt8(bytes));
assert(size <= bytes.size());
- putBytes(BytesView{bytes.data(), size});
+ putBytes(BytesView {bytes.data(), size});
Review Comment:
I'm not sure there should be spaces according to new coding style.
##########
modules/platforms/cpp/schema/BinaryTupleParser.cpp:
##########
@@ -18,22 +18,61 @@
#include "BinaryTupleParser.h"
#include "common/Platform.h"
+#include "common/Bytes.h"
+
#include <cassert>
#include <cstring>
#include <stdexcept>
+namespace ignite {
+
namespace {
template <typename T>
-T as(ignite::BytesView bytes) noexcept {
- T value;
- std::memcpy(&value, bytes.data(), sizeof(T));
- return value;
+T loadBytesAs(BytesView bytes, std::size_t offset = 0) noexcept {
+ return bytes::load<Endian::LITTLE, T>(bytes.data() + offset);
}
-} // namespace
+Date loadBytesAsDate(ignite::BytesView bytes) {
+ std::int32_t date = loadBytesAs<std::uint16_t>(bytes);
+ date |= std::int32_t(loadBytesAs<std::int8_t>(bytes, 2)) << 16;
-namespace ignite {
+ std::int32_t day = date & 31;
+ std::int32_t month = (date >> 5) & 15;
+ std::int32_t year = (date >> 9); // Sign matters.
+
+ return Date(year, month, day);
+}
+
+Time loadBytesAsTime(ignite::BytesView bytes) {
+ uint64_t time = loadBytesAs<std::uint32_t>(bytes);
+
+ int nano;
+ switch (bytes.size()) {
+ case 4:
+ nano = ((int)time & ((1 << 10) - 1)) * 1000 * 1000;
+ time >>= 10;
+ break;
+ case 5:
+ time |= std::uint64_t(loadBytesAs<std::uint8_t>(bytes, 4)) << 32;
+ nano = ((int)time & ((1 << 20) - 1)) * 1000;
+ time >>= 20;
+ break;
+ case 6:
+ time |= std::uint64_t(loadBytesAs<std::uint16_t>(bytes, 4)) << 32;
+ nano = ((int)time & ((1 << 30) - 1));
+ time >>= 30;
+ break;
+ }
+
+ int second = ((int)time) & 63;
Review Comment:
I believe hex (i.e. `0x3F` or `0x40 - 1`) for bit masks is more common
approach.
Also, applying this kind of mask to `int` value implies there can be no
negative values. Why use `int` then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
