Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,17 @@ else()
message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for MariaDBClientCpp")
endif()

# Find and setup msgpack
if(SPIDER_USE_STATIC_LIBS)
set(msgpack-cxx_USE_STATIC_LIBS ON)
endif()
find_package(msgpack-cxx 7.0.0 REQUIRED)
if(msgpack-cxx_FOUND)
message(STATUS "Found msgpack-cxx ${msgpack-cxx_VERSION}")
else()
message(FATAL_ERROR "Could not find msgpack-cxx")
endif()
Comment on lines +134 to +143

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling consistency with other dependencies

The error handling for msgpack-cxx should follow the same pattern as other dependencies in the file, particularly for static/shared library scenarios.

Apply this diff to improve the error handling:

 # Find and setup msgpack
 if(SPIDER_USE_STATIC_LIBS)
     set(msgpack-cxx_USE_STATIC_LIBS ON)
 endif()
 find_package(msgpack-cxx 7.0.0 REQUIRED)
 if(msgpack-cxx_FOUND)
     message(STATUS "Found msgpack-cxx ${msgpack-cxx_VERSION}")
 else()
-    message(FATAL_ERROR "Could not find msgpack-cxx")
+    if(SPIDER_USE_STATIC_LIBS)
+        message(FATAL_ERROR "Could not find static libraries for msgpack-cxx")
+    else()
+        message(FATAL_ERROR "Could not find ${SPIDER_LIBS_STRING} libraries for msgpack-cxx")
+    endif()
 endif()

Committable suggestion skipped: line range outside the PR's diff.


# Add abseil-cpp
set(ABSL_PROPAGATE_CXX_STD ON)
add_subdirectory(submodules/abseil-cpp)
Expand Down
3 changes: 3 additions & 0 deletions src/spider/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ set(SPIDER_CORE_HEADERS
core/Data.hpp
core/Task.hpp
core/TaskGraph.hpp
core/Serializer.hpp
storage/MetadataStorage.hpp
storage/DataStorage.hpp
storage/MysqlStorage.hpp
worker/FunctionManager.hpp
CACHE INTERNAL
"spider core header files"
)
Expand All @@ -22,6 +24,7 @@ target_link_libraries(
Boost::boost
absl::flat_hash_map
MariaDBClientCpp::MariaDBClientCpp
msgpack-cxx
)
target_link_libraries(spider_core PRIVATE fmt::fmt)

Expand Down
10 changes: 10 additions & 0 deletions src/spider/core/Data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,14 @@
#include <utility>
#include <vector>

#include "MsgPack.hpp" // IWYU pragma: keep
#include "Serializer.hpp" // IWYU pragma: keep

namespace spider::core {
class Data {
public:
Data() { init_id(); }

explicit Data(std::string value) : m_value(std::move(value)) { init_id(); }

Data(boost::uuids::uuid id, std::string value) : m_id(id), m_value(std::move(value)) {}
Expand All @@ -24,6 +29,10 @@ class Data {
m_key(std::move(key)),
m_value(std::move(value)) {}

MSGPACK_DEFINE(m_id, m_key, m_value, m_locality, m_hard_locality);

static auto is_data() -> bool { return true; }

[[nodiscard]] auto get_id() const -> boost::uuids::uuid { return m_id; }

[[nodiscard]] auto get_key() const -> std::optional<std::string> { return m_key; }
Expand Down Expand Up @@ -52,6 +61,7 @@ class Data {
m_id = gen();
}
};

} // namespace spider::core

#endif // SPIDER_CORE_DATA_HPP
35 changes: 35 additions & 0 deletions src/spider/core/MsgPack.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#ifndef SPIDER_MSGPACK_HPP
#define SPIDER_MSGPACK_HPP

// clang-format off
// IWYU pragma: begin_exports

#include <msgpack.hpp>

#include <msgpack/iterator.hpp>
#include <msgpack/null_visitor.hpp>
#include <msgpack/object.hpp>
#include <msgpack/pack.hpp>
#include <msgpack/parse.hpp>
#include <msgpack/sbuffer.hpp>
#include <msgpack/type.hpp>
#include <msgpack/unpack.hpp>
#include <msgpack/version.hpp>
#include <msgpack/vrefbuffer.hpp>
#include <msgpack/x3_parse.hpp>
#include <msgpack/x3_unpack.hpp>
#include <msgpack/zone.hpp>

#include <msgpack/adaptor/define_decl.hpp>
#include <msgpack/v3/adaptor/adaptor_base.hpp>
#include <msgpack/v3/object_decl.hpp>
#include <msgpack/v3/object_fwd.hpp>
#include <msgpack/v3/object_fwd_decl.hpp>
#include <msgpack/v3/pack_decl.hpp>
#include <msgpack/v3/sbuffer_decl.hpp>
#include <msgpack/v3/unpack.hpp>

// IWYU pragma: end_exports
// clang-format on

#endif // SPIDER_MSGPACK_HPP
43 changes: 43 additions & 0 deletions src/spider/core/Serializer.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#ifndef SPIDER_CORE_SERIALIZER_HPP
#define SPIDER_CORE_SERIALIZER_HPP

#include <boost/uuid/uuid.hpp>
#include <cstdint>
#include <cstring>

#include "MsgPack.hpp" // IWYU pragma: keep

template <>
struct msgpack::adaptor::convert<boost::uuids::uuid> {
auto operator()(msgpack::object const& object, boost::uuids::uuid& id) const
-> msgpack::object const& {
Comment on lines +12 to +13

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider returning void instead of const reference.

Returning a const reference to a parameter can lead to dangling references if the parameter's lifetime ends. Since the object isn't modified, consider changing the return type to void.

-    auto operator()(msgpack::object const& object, boost::uuids::uuid& id) const
-            -> msgpack::object const& {
+    void operator()(msgpack::object const& object, boost::uuids::uuid& id) const {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
auto operator()(msgpack::object const& object, boost::uuids::uuid& id) const
-> msgpack::object const& {
void operator()(msgpack::object const& object, boost::uuids::uuid& id) const {

// NOLINTBEGIN(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays,cppcoreguidelines-pro-bounds-array-to-pointer-decay,bugprone-return-const-ref-from-parameter)
if (object.type != type::BIN) {
throw type_error();
}
if (object.via.bin.size != boost::uuids::uuid::static_size()) {
throw type_error();
}
std::uint8_t data[boost::uuids::uuid::static_size()];
std::memcpy(data, object.via.bin.ptr, boost::uuids::uuid::static_size());
id = boost::uuids::uuid{data};
Comment on lines +21 to +23

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using std::array for safer memory operations.

The current implementation uses C-style arrays and memcpy. Consider using std::array for better type safety and std::copy_n for more explicit bounds checking.

-        std::uint8_t data[boost::uuids::uuid::static_size()];
-        std::memcpy(data, object.via.bin.ptr, boost::uuids::uuid::static_size());
-        id = boost::uuids::uuid{data};
+        std::array<std::uint8_t, boost::uuids::uuid::static_size()> data;
+        std::copy_n(static_cast<const std::uint8_t*>(object.via.bin.ptr),
+                    boost::uuids::uuid::static_size(),
+                    data.begin());
+        id = boost::uuids::uuid{data.data()};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::uint8_t data[boost::uuids::uuid::static_size()];
std::memcpy(data, object.via.bin.ptr, boost::uuids::uuid::static_size());
id = boost::uuids::uuid{data};
std::array<std::uint8_t, boost::uuids::uuid::static_size()> data;
std::copy_n(static_cast<const std::uint8_t*>(object.via.bin.ptr),
boost::uuids::uuid::static_size(),
data.begin());
id = boost::uuids::uuid{data.data()};


return object;
// NOLINTEND(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays,cppcoreguidelines-pro-bounds-array-to-pointer-decay,bugprone-return-const-ref-from-parameter)
}
};

template <>
struct msgpack::adaptor::pack<boost::uuids::uuid> {
template <class Stream>
auto operator()(msgpack::packer<Stream>& packer, boost::uuids::uuid const& id) const
-> msgpack::packer<Stream>& {
packer.pack_bin(id.size());
// NOLINTBEGIN(cppcoreguidelines-pro-type-cstyle-cast)
packer.pack_bin_body((char const*)id.data(), id.size());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace C-style cast with reinterpret_cast.

Using C-style casts can hide dangerous conversions. Use explicit C++ cast operators instead.

-        packer.pack_bin_body((char const*)id.data(), id.size());
+        packer.pack_bin_body(reinterpret_cast<char const*>(id.data()), id.size());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
packer.pack_bin_body((char const*)id.data(), id.size());
packer.pack_bin_body(reinterpret_cast<char const*>(id.data()), id.size());

// NOLINTEND(cppcoreguidelines-pro-type-cstyle-cast)
return packer;
}
};

#endif // SPIDER_CORE_SERIALIZER_HPP
Loading