Fix Serialisation to properly deal with trivially copyable types

docs/networking.rst

@@ -43,9 +43,8 @@ checks explicitly for an explicit type. Be careful about multiple declarations.
 For this partial specialisation three static methods need to be defined.
 
 .. codeblock:: c++
-    static inline std::vector<char> serialise(const T& in)
+    static inline std::vector<uint8_t> serialise(const T& in)
 
-    static inline T deserialise(const std::vector<char>& in)
+    static inline T deserialise(const std::vector<uint8_t>& in)
 
     static inline uint64_t hash()
-

src/PowerPlant.cpp

@@ -24,7 +24,8 @@
 
 namespace NUClear {
 
-PowerPlant* PowerPlant::powerplant = nullptr;  // NOLINT
+// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
+PowerPlant* PowerPlant::powerplant = nullptr;
 
 PowerPlant::~PowerPlant() {
     // Make sure reactors are destroyed before anything else

src/dsl/word/Network.hpp

@@ -96,7 +96,7 @@ namespace dsl {
             static inline std::tuple<std::shared_ptr<NetworkSource>, NetworkData<T>> get(
                 const threading::Reaction& /*reaction*/) {
 
-                auto* data   = store::ThreadStore<std::vector<char>>::value;
+                auto* data   = store::ThreadStore<std::vector<uint8_t>>::value;
                 auto* source = store::ThreadStore<NetworkSource>::value;
 
                 if (data && source) {

src/dsl/word/UDP.hpp

@@ -91,7 +91,7 @@ namespace dsl {
                 /// @brief If the packet is valid
                 bool valid{false};
                 /// @brief The data that was received
-                std::vector<char> payload{};
+                std::vector<uint8_t> payload{};
                 /// @brief The local address that the packet was received on
                 util::network::sock_t local{};
                 /// @brief The remote address that the packet was received from
@@ -121,7 +121,7 @@ namespace dsl {
                 Target remote;
 
                 /// @brief The data to be sent in the packet
-                std::vector<char> payload{};
+                std::vector<uint8_t> payload{};
 
                 /**
                  * @brief Casts this packet to a boolean to check if it is valid
@@ -350,13 +350,13 @@ namespace dsl {
                 }
 
                 // Allocate max size for a UDP packet
-                std::vector<char> buffer(65535, 0);
+                std::vector<uint8_t> buffer(65535, 0);
 
                 // Make some variables to hold our message header information
                 std::array<char, 0x100> cmbuff = {0};
                 util::network::sock_t remote{};
                 iovec payload{};
-                payload.iov_base = buffer.data();
+                payload.iov_base = reinterpret_cast<char*>(buffer.data());
                 payload.iov_len  = static_cast<decltype(payload.iov_len)>(buffer.size());
 
                 // Make our message header to receive with

src/dsl/word/emit/Network.hpp

@@ -39,7 +39,7 @@ namespace dsl {
                 /// The hash identifying the type of object
                 uint64_t hash{0};
                 /// The serialised data
-                std::vector<char> payload{};
+                std::vector<uint8_t> payload{};
                 /// If the message should be sent reliably
                 bool reliable{false};
             };

src/dsl/word/emit/UDP.hpp

@@ -157,11 +157,11 @@ namespace dsl {
                     }
 
                     // Serialise to our payload
-                    std::vector<char> payload = util::serialise::Serialise<DataType>::serialise(*data);
+                    std::vector<uint8_t> payload = util::serialise::Serialise<DataType>::serialise(*data);
 
                     // Try to send our payload
                     if (::sendto(fd,
-                                 payload.data(),
+                                 reinterpret_cast<const char*>(payload.data()),
                                  static_cast<socklen_t>(payload.size()),
                                  0,
                                  &remote.sock,

src/extension/NetworkController.hpp

@@ -51,15 +51,15 @@ namespace extension {
             network.set_packet_callback([this](const network::NUClearNetwork::NetworkTarget& remote,
                                                const uint64_t& hash,
                                                const bool& reliable,
-                                               std::vector<char>&& payload) {
+                                               std::vector<uint8_t>&& payload) {
                 // Construct our NetworkSource information
                 dsl::word::NetworkSource src{remote.name, remote.target, reliable};
 
                 // Move the payload in as we are stealing it
-                std::vector<char> p(std::move(payload));
+                std::vector<uint8_t> p(std::move(payload));
 
                 // Store in our thread local cache
-                dsl::store::ThreadStore<std::vector<char>>::value        = &p;
+                dsl::store::ThreadStore<std::vector<uint8_t>>::value        = &p;
                 dsl::store::ThreadStore<dsl::word::NetworkSource>::value = &src;
 
                 /* Mutex Scope */ {
@@ -76,7 +76,7 @@ namespace extension {
                 }
 
                 // Clear our cache
-                dsl::store::ThreadStore<std::vector<char>>::value        = nullptr;
+                dsl::store::ThreadStore<std::vector<uint8_t>>::value        = nullptr;
                 dsl::store::ThreadStore<dsl::word::NetworkSource>::value = nullptr;
             });
 

src/extension/network/NUClearNetwork.cpp

@@ -46,12 +46,12 @@ namespace extension {
          *
          * @return the data and who it was sent from
          */
-        std::pair<util::network::sock_t, std::vector<char>> read_socket(fd_t fd) {
+        std::pair<util::network::sock_t, std::vector<uint8_t>> read_socket(fd_t fd) {
 
             // Allocate a vector that can hold a datagram
-            std::vector<char> payload(1500);
+            std::vector<uint8_t> payload(1500);
             iovec iov{};
-            iov.iov_base = payload.data();
+            iov.iov_base = reinterpret_cast<char*>(payload.data());
             iov.iov_len  = static_cast<decltype(iov.iov_len)>(payload.size());
 
             // Who we are receiving from
@@ -82,7 +82,7 @@ namespace extension {
         }
 
         void NUClearNetwork::set_packet_callback(
-            std::function<void(const NetworkTarget&, const uint64_t&, const bool&, std::vector<char>&&)> f) {
+            std::function<void(const NetworkTarget&, const uint64_t&, const bool&, std::vector<uint8_t>&&)> f) {
             packet_callback = std::move(f);
         }
 
@@ -534,7 +534,7 @@ namespace extension {
 
                 // Send the packet
                 if (::sendto(data_fd,
-                             announce_packet.data(),
+                             reinterpret_cast<const char*>(announce_packet.data()),
                              static_cast<socklen_t>(announce_packet.size()),
                              0,
                              &it->second->target.sock,
@@ -547,11 +547,11 @@ namespace extension {
             }
         }
 
-        void NUClearNetwork::process_packet(const sock_t& address, std::vector<char>&& payload) {
+        void NUClearNetwork::process_packet(const sock_t& address, std::vector<uint8_t>&& payload) {
 
             // First validate this is a NUClear network packet we can read (a version 2 NUClear packet)
-            if (payload.size() >= sizeof(PacketHeader) && payload[0] == '\xE2' && payload[1] == '\x98'
-                && payload[2] == '\xA2' && payload[3] == 0x02) {
+            if (payload.size() >= sizeof(PacketHeader) && payload[0] == 0xE2 && payload[1] == 0x98 && payload[2] == 0xA2
+                && payload[3] == 0x02) {
 
                 // This is a real packet! get our header information
                 const PacketHeader& header = *reinterpret_cast<const PacketHeader*>(payload.data());
@@ -595,7 +595,7 @@ namespace extension {
 
                                         // Say hi back!
                                         ::sendto(data_fd,
-                                                 announce_packet.data(),
+                                                 reinterpret_cast<const char*>(announce_packet.data()),
                                                  static_cast<socklen_t>(announce_packet.size()),
                                                  0,
                                                  &ptr->target.sock,
@@ -671,7 +671,7 @@ namespace extension {
                                 if (it != remote->recent_packets.end() && packet.reliable) {
 
                                     // Allocate room for the whole ack packet
-                                    std::vector<char> r(sizeof(ACKPacket) + (packet.packet_count / 8), 0);
+                                    std::vector<uint8_t> r(sizeof(ACKPacket) + (packet.packet_count / 8), 0);
                                     ACKPacket& response   = *reinterpret_cast<ACKPacket*>(r.data());
                                     response              = ACKPacket();
                                     response.packet_id    = packet.packet_id;
@@ -688,7 +688,7 @@ namespace extension {
 
                                     // Send the packet
                                     ::sendto(data_fd,
-                                             r.data(),
+                                             reinterpret_cast<const char*>(r.data()),
                                              static_cast<socklen_t>(r.size()),
                                              0,
                                              &to.sock,
@@ -703,8 +703,8 @@ namespace extension {
                             if (packet.packet_count == 1) {
 
                                 // Copy our data into a vector
-                                std::vector<char> out(&packet.data,
-                                                      &packet.data + payload.size() - sizeof(DataPacket) + 1);
+                                std::vector<uint8_t> out(&packet.data,
+                                                         &packet.data + payload.size() - sizeof(DataPacket) + 1);
 
                                 // If this is a reliable packet, send an ack back
                                 if (packet.reliable) {
@@ -751,7 +751,7 @@ namespace extension {
 
                                         // A basic ack has room for 8 packets and we need 1 extra byte for each 8
                                         // additional packets
-                                        std::vector<char> r(sizeof(NACKPacket) + (packet.packet_count / 8), 0);
+                                        std::vector<uint8_t> r(sizeof(NACKPacket) + (packet.packet_count / 8), 0);
                                         NACKPacket& response  = *reinterpret_cast<NACKPacket*>(r.data());
                                         response              = NACKPacket();
                                         response.packet_id    = packet.packet_id;
@@ -771,7 +771,7 @@ namespace extension {
 
                                         // Send the packet
                                         ::sendto(data_fd,
-                                                 r.data(),
+                                                 reinterpret_cast<const char*>(r.data()),
                                                  static_cast<socklen_t>(r.size()),
                                                  0,
                                                  &to.sock,
@@ -790,7 +790,7 @@ namespace extension {
                                 if (packet.reliable) {
                                     // A basic ack has room for 8 packets and we need 1 extra byte for each 8
                                     // additional packets
-                                    std::vector<char> r(sizeof(ACKPacket) + (packet.packet_count / 8), 0);
+                                    std::vector<uint8_t> r(sizeof(ACKPacket) + (packet.packet_count / 8), 0);
                                     ACKPacket& response   = *reinterpret_cast<ACKPacket*>(r.data());
                                     response              = ACKPacket();
                                     response.packet_id    = packet.packet_id;
@@ -807,7 +807,7 @@ namespace extension {
 
                                     // Send the packet
                                     ::sendto(data_fd,
-                                             r.data(),
+                                             reinterpret_cast<const char*>(r.data()),
                                              static_cast<socklen_t>(r.size()),
                                              0,
                                              &to.sock,
@@ -825,7 +825,7 @@ namespace extension {
                                     }
 
                                     // Read in our data
-                                    std::vector<char> out;
+                                    std::vector<uint8_t> out;
                                     out.reserve(payload_size);
                                     for (auto& p : assembler.second) {
                                         const DataPacket& part = *reinterpret_cast<DataPacket*>(p.second.data());
@@ -997,7 +997,7 @@ namespace extension {
         void NUClearNetwork::send_packet(const sock_t& target,
                                          NUClear::extension::network::DataPacket header,
                                          uint16_t packet_no,
-                                         const std::vector<char>& payload,
+                                         const std::vector<uint8_t>& payload,
                                          const bool& /*reliable*/) {
 
             // Our packet we are sending
@@ -1012,13 +1012,14 @@ namespace extension {
             data[0].iov_base = reinterpret_cast<char*>(&header);
             data[0].iov_len  = sizeof(DataPacket) - 1;
 
-            // Work out what chunk of data we are sending const cast is fine as posix guarantees it won't be
-            // modified
-            data[1].iov_base = const_cast<char*>(payload.data() + (packet_data_mtu * packet_no));  // NOLINT
-            data[1].iov_len  = packet_no + 1 < header.packet_count ? packet_data_mtu : payload.size() % packet_data_mtu;
+            // Work out what chunk of data we are sending
+            // const cast is fine as posix guarantees it won't be modified on a sendmsg
+            const char* start = reinterpret_cast<const char*>(payload.data()) + (packet_no * packet_data_mtu);
+            data[1].iov_base  = const_cast<char*>(start);  // NOLINT(cppcoreguidelines-pro-type-const-cast)
+            data[1].iov_len = packet_no + 1 < header.packet_count ? packet_data_mtu : payload.size() % packet_data_mtu;
 
             // Set our target and send (once again const cast is fine)
-            message.msg_name    = const_cast<sockaddr*>(&target.sock);  // NOLINT
+            message.msg_name    = const_cast<sockaddr*>(&target.sock);  // NOLINT(cppcoreguidelines-pro-type-const-cast)
             message.msg_namelen = target.size();
 
             // TODO(trent): if reliable, run select first to see if this socket is writeable
@@ -1028,7 +1029,7 @@ namespace extension {
 
 
         void NUClearNetwork::send(const uint64_t& hash,
-                                  const std::vector<char>& payload,
+                                  const std::vector<uint8_t>& payload,
                                   const std::string& target,
                                   bool reliable) {
 

src/extension/network/NUClearNetwork.hpp

@@ -75,7 +75,7 @@ namespace extension {
                 std::mutex assemblers_mutex;
                 /// Storage for fragmented packets while we build them
                 std::map<uint16_t,
-                         std::pair<std::chrono::steady_clock::time_point, std::map<uint16_t, std::vector<char>>>>
+                         std::pair<std::chrono::steady_clock::time_point, std::map<uint16_t, std::vector<uint8_t>>>>
                     assemblers{};
 
                 /// Struct storing the kalman filter for round trip time
@@ -130,15 +130,15 @@ namespace extension {
              * @param target        who we are sending to (blank means everyone)
              * @param reliable      if the delivery of the data should be ensured
              */
-            void send(const uint64_t& hash, const std::vector<char>& payload, const std::string& target, bool reliable);
+            void send(const uint64_t& hash, const std::vector<uint8_t>& payload, const std::string& target, bool reliable);
 
             /**
              * @brief Set the callback to use when a data packet is completed
              *
              * @param f the callback function
              */
             void set_packet_callback(
-                std::function<void(const NetworkTarget&, const uint64_t&, const bool&, std::vector<char>&&)> f);
+                std::function<void(const NetworkTarget&, const uint64_t&, const bool&, std::vector<uint8_t>&&)> f);
 
             /**
              * @brief Set the callback to use when a node joins the network
@@ -231,7 +231,7 @@ namespace extension {
                 DataPacket header{};
 
                 /// The data to send
-                std::vector<char> payload{};
+                std::vector<uint8_t> payload{};
             };
 
             /**
@@ -255,7 +255,7 @@ namespace extension {
              * @param address   who the packet came from
              * @param data      the data that was sent in this packet
              */
-            void process_packet(const sock_t& address, std::vector<char>&& payload);
+            void process_packet(const sock_t& address, std::vector<uint8_t>&& payload);
 
             /**
              * @brief Send an announce packet to our announce address
@@ -279,7 +279,7 @@ namespace extension {
             void send_packet(const sock_t& target,
                              DataPacket header,
                              uint16_t packet_no,
-                             const std::vector<char>& payload,
+                             const std::vector<uint8_t>& payload,
                              const bool& reliable);
 
             /**
@@ -307,13 +307,13 @@ namespace extension {
             uint16_t packet_data_mtu{1000};
 
             // Our announce packet
-            std::vector<char> announce_packet{};
+            std::vector<uint8_t> announce_packet{};
 
             /// An atomic source for packet IDs to make sure they are semi unique
             std::atomic<uint16_t> packet_id_source{0};
 
             /// The callback to execute when a data packet is completed
-            std::function<void(const NetworkTarget&, const uint64_t&, const bool&, std::vector<char>&&)>
+            std::function<void(const NetworkTarget&, const uint64_t&, const bool&, std::vector<uint8_t>&&)>
                 packet_callback;
             /// The callback to execute when a node joins the network
             std::function<void(const NetworkTarget&)> join_callback;

src/id.hpp

@@ -27,9 +27,7 @@
 
 namespace NUClear {
 
-/**
- * @brief A unique identifier for a thread pool
- */
+/// @brief This type is used when NUClear requires a unique identifier
 using id_t = std::size_t;
 
 }  // namespace NUClear

src/threading/ReactionTask.hpp

@@ -28,6 +28,7 @@
 #include <memory>
 #include <typeindex>
 #include <vector>
+
 #include "../id.hpp"
 #include "../message/ReactionStatistics.hpp"
 #include "../util/GroupDescriptor.hpp"
@@ -151,11 +152,13 @@ namespace threading {
 
     // Initialize our id source
     template <typename ReactionType>
-    std::atomic<NUClear::id_t> Task<ReactionType>::task_id_source(0);  // NOLINT
+    // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
+    std::atomic<NUClear::id_t> Task<ReactionType>::task_id_source(0);
 
     // Initialize our current task
     template <typename ReactionType>
-    ATTRIBUTE_TLS Task<ReactionType>* Task<ReactionType>::current_task = nullptr;  // NOLINT
+    // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
+    ATTRIBUTE_TLS Task<ReactionType>* Task<ReactionType>::current_task = nullptr;
 
     // Alias the templated Task so that public API remains intact
     class Reaction;