Refactor PBufWrapper move/copy implementations

PBuf reference counting is done inside LwIP. PBufWrapper should be transparent implementation without intrusive reference counting. This commit makes `PBufWrapper` class movable and non-copyable. Signed-off-by: Alexander Livenets <alexander.livenets@apex.ai>

Author: apexai-alex

include/rtps/communication/PacketInfo.h

@@ -45,11 +45,7 @@ struct PacketInfo {
  PacketInfo() = default;
  ~PacketInfo() = default;
 
- PacketInfo &operator=(const PacketInfo &other) {
- copyTriviallyCopyable(other);
- this->buffer = other.buffer;
- return *this;
- }
+ PacketInfo &operator=(const PacketInfo &other) = delete;
 
  PacketInfo &operator=(PacketInfo &&other) noexcept {
  copyTriviallyCopyable(other);

include/rtps/messages/MessageFactory.h

@@ -133,8 +133,7 @@ void addSubMessageData(Buffer &buffer, const Buffer &filledPayload,
  serializeMessage(buffer, msg);
 
  if (filledPayload.isValid()) {
- Buffer shallowCopy = filledPayload;
- buffer.append(std::move(shallowCopy));
+ buffer.append(filledPayload);
  }
 }
 

include/rtps/storages/PBufWrapper.h

@@ -38,28 +38,26 @@ struct PBufWrapper {
  explicit PBufWrapper(pbuf *bufferToWrap);
  explicit PBufWrapper(DataSize_t length);
 
- // Shallow Copy. No copying of the underlying pbuf. Just another reference
- // like a shared pointer.
- PBufWrapper(const PBufWrapper &other);
- PBufWrapper &operator=(const PBufWrapper &other);
+ PBufWrapper(const PBufWrapper &other) = delete;
+ PBufWrapper &operator=(const PBufWrapper &other) = delete;
 
  PBufWrapper(PBufWrapper &&other) noexcept;
  PBufWrapper &operator=(PBufWrapper &&other) noexcept;
 
  ~PBufWrapper();
 
- PBufWrapper deepCopy() const;
-
  bool isValid() const;
 
  bool append(const uint8_t *data, DataSize_t length);
 
  /// Note that unused reserved memory is now part of the wrapper. New calls to
  /// append(uint8_t*[...]) will continue behind the appended wrapper
- void append(PBufWrapper &&other);
+ void append(const PBufWrapper &other);
 
  bool reserve(DataSize_t length);
 
+ void destroy();
+
  /// After calling this function, data is added starting from the beginning
  /// again. It does not revert reserve.
  void reset();

src/storages/PBufWrapper.cpp

@@ -51,24 +51,11 @@ PBufWrapper::PBufWrapper(DataSize_t length)
  }
 }
 
-// TODO: Uses copy assignment. Improvement possible
-PBufWrapper::PBufWrapper(const PBufWrapper &other) { *this = other; }
-
 // TODO: Uses move assignment. Improvement possible
 PBufWrapper::PBufWrapper(PBufWrapper &&other) noexcept {
  *this = std::move(other);
 }
 
-PBufWrapper &PBufWrapper::operator=(const PBufWrapper &other) {
- copySimpleMembersAndResetBuffer(other);
-
- if (other.firstElement != nullptr) {
- pbuf_ref(other.firstElement);
- }
- firstElement = other.firstElement;
- return *this;
-}
-
 PBufWrapper &PBufWrapper::operator=(PBufWrapper &&other) noexcept {
  copySimpleMembersAndResetBuffer(other);
 
@@ -88,26 +75,17 @@ void PBufWrapper::copySimpleMembersAndResetBuffer(const PBufWrapper &other) {
  }
 }
 
-PBufWrapper::~PBufWrapper() {
+void PBufWrapper::destroy()
+{
  if (firstElement != nullptr) {
  pbuf_free(firstElement);
+ firstElement = nullptr;
  }
+ m_freeSpace = 0;
 }
 
-PBufWrapper PBufWrapper::deepCopy() const {
- PBufWrapper clone;
- clone.copySimpleMembersAndResetBuffer(*this);
-
- // Decided not to use pbuf_clone because it prevents const
- clone.firstElement = pbuf_alloc(m_layer, this->firstElement->tot_len, m_type);
- if (clone.firstElement != nullptr) {
- if (pbuf_copy(clone.firstElement, this->firstElement) != ERR_OK) {
- PBUF_WRAP_LOG("PBufWrapper::deepCopy: Copy of pbuf failed");
- }
- } else {
- clone.m_freeSpace = 0;
- }
- return clone;
+PBufWrapper::~PBufWrapper() {
+ destroy();
 }
 
 bool PBufWrapper::isValid() const { return firstElement != nullptr; }
@@ -131,29 +109,24 @@ bool PBufWrapper::append(const uint8_t *data, DataSize_t length) {
  if (err != ERR_OK) {
  return false;
  }
-
  m_freeSpace -= length;
  return true;
 }
 
-void PBufWrapper::append(PBufWrapper &&other) {
- if (this == &other) {
- return;
- }
+void PBufWrapper::append(const PBufWrapper &other) {
  if (this->firstElement == nullptr) {
- *this = std::move(other);
+ m_freeSpace = other.m_freeSpace;
+ this->firstElement = other.firstElement;
+ pbuf_ref(this->firstElement);
  return;
  }
 
- m_freeSpace = other.m_freeSpace;
- pbuf *const newElement = other.firstElement;
- pbuf_cat(this->firstElement, newElement);
-
- other.firstElement = nullptr;
+ m_freeSpace += other.m_freeSpace;
+ pbuf_chain(this->firstElement, other.firstElement);
 }
 
 bool PBufWrapper::reserve(DataSize_t length) {
- auto additionalAllocation = length - m_freeSpace;
+ int16_t additionalAllocation = length - m_freeSpace;
  if (additionalAllocation <= 0) {
  return true;
  }