more more more

Author: ibc

TODO_SCTP.md

@@ -48,26 +48,19 @@ else if (auto* barItem = std::get_if<std::unique_ptr<BarItem>>(&item))
 // However we can check item->GetType() instead and do a switch().
 ```
 
-## Parsing of subclasses FooItem
-
-- Must initialize Value Length field in the constructor.
-- Must provide with a `virtual bool FooItem::Verify() const = 0` method in which subclasses verify if header, flags, value length and value is ok.
-- Must probably remove `GetValue()` and `SetValue()` in parent `FooItem` class or make then protected.
-
-NO! Instead:
-
-- Must remove static `Parse()` and `Factory()` in FooItem and implement
-- them into each subclass.
-
 ## Optimize creation of packets with items
 
 - Add some `GetNextItemPtr(size_t& remainingBufferLength)` in a packert class that returns a pointer to the position in which a new item would take place and modifies the given `remainingBufferLength` with the remaining length of the buffer after that position.
 - Then create a `Item` via `Item::Factory()` by passing `ptr` as `buffer` and `remainingBufferLength` as `bufferLength`.
 - Then also add `packet->AddInPlaceItem(item, ptr)` (or whatever) so the packet doesn't serialize (doesn't memcpy) the bytes of the `Item` into its buffer.
 - Another related option is to add specialized `AddXxxxItem()` methods in the packet class.
 
-## FooItem subclasses
+## FooPacket and FooItem TODO
 
-- If we have a FooDataItem, the app should not be able to change its id via `SetId()`, AKA make `SetId()` protected since app should use `FooXxxxItem::Factory()` static methods.
-- `FooItem` constructor: It doesn't make sense that it fills things since we want to rely on subclasses' `Parse()`, `Factory()` and constructors (that will override these calls anyway).
 - If `FooItem` has valid but unknown id, that's correct, but let's remove the `SetValue()` method so those unknown items can only be parsed but not created.
+- Remove `FooItem::GetValue()` and `GetValueLength()`.
+- Document that having `auto item = FooPacket::GetIem(idx)`, the caller needs to cast to specific `FooItem` subclass by checking `item->GetId()` and then doing this, HOEWEVER this will **invalidate** the `FooItem` in the `FooPacket`!!!:
+  ```c++
+  auto numericItem = std::unique_ptr<FooNumericItem>(static_cast<FooNumericItem*>(item.release()));
+  ```
+- Wait!

worker/include/common.hpp

@@ -6,7 +6,7 @@
 #include <cstddef>    // size_t
 #include <cstdint>    // uint8_t, etc
 #include <functional> // std::function
-#include <memory>     // std::addressof()
+#include <memory>     // std::addressof(), std::unique_ptr(), etc
 #include <optional>
 #ifdef _WIN32
 #include <winsock2.h>

worker/meson.build

@@ -390,7 +390,9 @@ if get_option('ms_sctp_stack')
     'test/src/RTC/TestSerializable.cpp',
     'test/src/RTC/TestSerializable/FooPacket.cpp',
     'test/src/RTC/TestSerializable/FooItem.cpp',
-    'test/src/RTC/TestSerializable/FooDataItem.cpp',
+    'test/src/RTC/TestSerializable/FooNumericItem.cpp',
+    'test/src/RTC/TestSerializable/FooTextItem.cpp',
+    'test/src/RTC/TestSerializable/FooUnknownItem.cpp',
   ]
 endif
 

worker/test/include/RTC/TestSerializable/FooItem.hpp

@@ -38,16 +38,12 @@ class FooItem : public Serializable
 	 * Item Id.
 	 *
 	 * @remarks
-	 * - NONE is not a valid value, so a FooItem with id NONE should be
-	 *   considered invalid and parsing should fail.
-	 * - Values other than the defined must be accepted as FooUnknownItem.
+	 * Values other than the defined must be accepted as FooUnknownItem.
 	 */
 	enum class ItemId : uint8_t
 	{
-		NONE    = 0x0,
-		DATA    = 0x1,
-		EVENT   = 0x2,
-		CONTROL = 0x3,
+		NUMERIC = 0x1,
+		TEXT    = 0x2
 	};
 
 	/**
@@ -75,30 +71,12 @@ class FooItem : public Serializable
 	 * @param buffer
 	 * @param bufferLength
 	 * @param itemId - If given buffer is a valid FooItem then `itemId` is
-	 *   rewritten to parsed ItemId. Otherwuise it's rewritten to ItemId::NONE
-	 *   (0).
+	 *   rewritten to parsed ItemId.
 	 * @param valueLength - If given buffer is a valid FooItem then `valueLength`
-	 *   is rewritten to the value of the Value Length field. Otherwise it's
-	 *   rewritten to 0.
+	 *   is rewritten to the value of the Value Length field.
 	 */
 	static bool IsFooItem(const uint8_t* buffer, size_t bufferLength, ItemId& itemId, uint8_t& valueLength);
 
-	/**
-	 * Parse a FooItem.
-	 *
-	 * @remarks
-	 * - `bufferLength` may exceed the exact length of the item.
-	 */
-	static std::unique_ptr<FooItem> Parse(const uint8_t* buffer, size_t bufferLength);
-
-	static std::unique_ptr<FooItem> Factory(
-	  uint8_t* buffer,
-	  size_t bufferLength,
-	  ItemId id,
-	  uint8_t flags,
-	  const uint8_t* value,
-	  uint8_t valueLength);
-
 	static const std::string& ItemId2String(ItemId id);
 
 private:
@@ -109,7 +87,7 @@ class FooItem : public Serializable
 	 * Constructor is protected because we only want to create FooItem instances
 	 * via Parse() and Factory() in subclasses.
 	 */
-	FooItem(const uint8_t* buffer, size_t bufferLength, bool initializeHeader);
+	FooItem(const uint8_t* buffer, size_t bufferLength);
 
 public:
 	virtual ~FooItem() override;
@@ -129,52 +107,17 @@ class FooItem : public Serializable
 		return GetHeaderPointer()->id;
 	}
 
-	virtual void SetId(ItemId id) final
-	{
-		GetHeaderPointer()->id = id;
-	}
-
 	virtual uint8_t GetFlags() const final
 	{
 		return GetHeaderPointer()->flags;
 	}
 
-	virtual void SetFlags(uint8_t flags) final
-	{
-		GetHeaderPointer()->flags = flags;
-	}
-
 	virtual bool HasValue() const final
 	{
 		return GetValueLengthField() > 0u;
 		;
 	}
 
-	// TODO: Make it protected since the app should use subclasses instead.
-	virtual const uint8_t* GetValue() const final
-	{
-		if (!HasValue())
-		{
-			return nullptr;
-		}
-
-		return GetValuePointer();
-	}
-
-	// TODO: Make it protected since the app should use subclasses instead.
-	virtual uint8_t GetValueLength() const final
-	{
-		if (!HasValue())
-		{
-			return 0u;
-		}
-
-		return GetValueLengthField();
-	}
-
-	// TODO: Make it protected since the app should use subclasses instead.
-	virtual void SetValue(const uint8_t* value, uint8_t valueLength) final;
-
 protected:
 	/**
 	 * NOTE: Return ItemHeader* instead of const ItemHeader* since we may
@@ -185,6 +128,13 @@ class FooItem : public Serializable
 		return reinterpret_cast<ItemHeader*>(const_cast<uint8_t*>(GetBuffer()));
 	}
 
+	void InitializeHeader(ItemId id, uint8_t flags, uint8_t valueLength)
+	{
+		GetHeaderPointer()->id    = id;
+		GetHeaderPointer()->flags = flags;
+		SetValueLengthField(valueLength);
+	}
+
 	/**
 	 * Private private because it returns the value of the Value Length field,
 	 * which is not useful for the application.
@@ -208,6 +158,16 @@ class FooItem : public Serializable
 		return const_cast<uint8_t*>(GetBuffer()) + ItemHeaderLength;
 	}
 
+	virtual uint8_t GetValueLength() const final
+	{
+		if (!HasValue())
+		{
+			return 0u;
+		}
+
+		return GetValueLengthField();
+	}
+
 	const uint8_t* GetEndPointer() const
 	{
 		return GetBuffer() + ItemHeaderLength + GetValueLength();

…ude/RTC/TestSerializable/FooDataItem.hpp …/RTC/TestSerializable/FooNumericItem.hppworker/test/include/RTC/TestSerializable/FooDataItem.hpp renamed to worker/test/include/RTC/TestSerializable/FooNumericItem.hpp worker/test/include/RTC/TestSerializable/FooDataItem.hpp renamed to worker/test/include/RTC/TestSerializable/FooNumericItem.hpp

@@ -1,58 +1,56 @@
-#ifndef MS_RTC_SERIALIZABLE_FOO_DATA_ITEM_HPP
-#define MS_RTC_SERIALIZABLE_FOO_DATA_ITEM_HPP
+#ifndef MS_RTC_SERIALIZABLE_FOO_NUMERIC_ITEM_HPP
+#define MS_RTC_SERIALIZABLE_FOO_NUMERIC_ITEM_HPP
 
 #include "common.hpp"
 #include "RTC/TestSerializable/FooItem.hpp"
 
 using namespace RTC;
 
 /**
- * FooDataItem.
+ * FooNumericItem.
  *
  *  0                   1                   2                   3
  *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  * |   Id  | Flags | Value Length  |            Number             |
  * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  *
- * - Id (4 bits): Unsigned integer.
+ * - Id (4 bits): Unsigned integer. Must be 1 (NUMERIC).
  * - Flags (4 bits).
  * - Value Length (8 bits): Length of the Value field. Must be 2.
  * - Number (16 bits): Unsigned integer.
  *
- * Length of a FooDataItem must be 4 bytes.
+ * Length of a FooNumericItem must be 4 bytes.
  */
 
-class FooDataItem : public FooItem
+class FooNumericItem : public FooItem
 {
 public:
 	/**
-	 * FooDataItem has fixed length.
+	 * FooNumericItem has fixed length.
 	 */
-	static const size_t Length{ 4u };
 	static const size_t NumberLength{ 2u };
 
 public:
 	/**
-	 * Parse a FooDataItem.
+	 * Parse a FooNumericItem.
 	 *
 	 * @remarks
 	 * - `bufferLength` may exceed the exact length of the item.
 	 */
-	static std::unique_ptr<FooDataItem> Parse(const uint8_t* buffer, size_t bufferLength);
+	static std::unique_ptr<FooNumericItem> Parse(const uint8_t* buffer, size_t bufferLength);
 
-	static std::unique_ptr<FooDataItem> Factory(
+	static std::unique_ptr<FooNumericItem> Factory(
 	  uint8_t* buffer, size_t bufferLength, uint8_t flags, uint16_t number);
 
 private:
 	/**
-	 * Constructor is private because we only want to create FooDataItem instances
-	 * via Parse() and Factory().
+	 * Private constructor used by Parse() and Factory() static methods.
 	 */
-	FooDataItem(const uint8_t* buffer, size_t bufferLength, bool initializeHeader);
+	FooNumericItem(const uint8_t* buffer, size_t bufferLength);
 
 public:
-	virtual ~FooDataItem() override;
+	virtual ~FooNumericItem() override;
 
 	virtual void Dump() const override final;
 

worker/test/include/RTC/TestSerializable/FooPacket.hpp

@@ -119,11 +119,11 @@ class FooPacket : public Serializable
 	{
 		if (HasAppendix())
 		{
-			return GetLengthField() > HeaderLength + AppendixLength;
+			return GetLengthField() > FooPacket::HeaderLength + FooPacket::AppendixLength;
 		}
 		else
 		{
-			return GetLengthField() > HeaderLength;
+			return GetLengthField() > FooPacket::HeaderLength;
 		}
 	}
 
@@ -132,13 +132,19 @@ class FooPacket : public Serializable
 		return this->items.size();
 	}
 
+	/**
+	 * Get the FooItem with index `idx` (starts at 0).
+	 */
 	const std::unique_ptr<FooItem>& GetItem(size_t idx) const;
 
+	template<typename T>
+	const T* GetItem(size_t idx) const;
+
 	/**
-	 * Serializes given Item into Packet's buffer.
+	 * Serializes given FooItem into Packet's buffer.
 	 *
 	 * @remarks
-	 * Once this method is called, the Item is owned by FooPacket instance.
+	 * Once this method is called, the FooItem is owned by FooPacket instance.
 	 */
 	void AddItem(std::unique_ptr<FooItem> item);
 
@@ -182,16 +188,16 @@ class FooPacket : public Serializable
 	 */
 	uint8_t* GetAppendixPointer() const
 	{
-		return const_cast<uint8_t*>(GetBuffer()) + HeaderLength;
+		return const_cast<uint8_t*>(GetBuffer()) + FooPacket::HeaderLength;
 	}
 
 	const uint8_t* GetItemsPointer() const
 	{
-		auto* ptr = GetBuffer() + HeaderLength;
+		auto* ptr = GetBuffer() + FooPacket::HeaderLength;
 
 		if (HasAppendix())
 		{
-			ptr += AppendixLength;
+			ptr += FooPacket::AppendixLength;
 		}
 
 		return ptr;
@@ -204,7 +210,7 @@ class FooPacket : public Serializable
 
 	/**
 	 * Must be used within Parse() static method (instead than AddItem()).
-	 * This method doesn't serializa the given Item into Packet's buffer since
+	 * This method doesn't serializa the given FooItem into Packet's buffer since
 	 * it's already serialized (obviously since we are parsing a buffer).
 	 */
 	void AddParsedItem(std::unique_ptr<FooItem> item)

worker/test/include/RTC/TestSerializable/FooTextItem.hpp

@@ -0,0 +1,60 @@
+#ifndef MS_RTC_SERIALIZABLE_FOO_TEXT_ITEM_HPP
+#define MS_RTC_SERIALIZABLE_FOO_TEXT_ITEM_HPP
+
+#include "common.hpp"
+#include "RTC/TestSerializable/FooItem.hpp"
+#include <string>
+#include <string_view>
+
+using namespace RTC;
+
+/**
+ * FooTextItem.
+ *
+ *  0                   1                   2                   3
+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |   Id  | Flags | Value Length  |            Text               |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |                              ...                              |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * - Id (4 bits): Unsigned integer. Must be 2 (TEXT).
+ * - Flags (4 bits).
+ * - Value Length (8 bits): Length of the Value field.
+ * - Text (variable length bits): String not ended in \0.
+ *
+ * Length of a FooTextItem is therefore variable.
+ */
+
+class FooTextItem : public FooItem
+{
+public:
+	/**
+	 * Parse a FooTextItem.
+	 *
+	 * @remarks
+	 * - `bufferLength` may exceed the exact length of the item.
+	 */
+	static std::unique_ptr<FooTextItem> Parse(const uint8_t* buffer, size_t bufferLength);
+
+	static std::unique_ptr<FooTextItem> Factory(
+	  uint8_t* buffer, size_t bufferLength, uint8_t flags, const std::string& text);
+
+private:
+	/**
+	 * Private constructor used by Parse() and Factory() static methods.
+	 */
+	FooTextItem(const uint8_t* buffer, size_t bufferLength);
+
+public:
+	virtual ~FooTextItem() override;
+
+	virtual void Dump() const override final;
+
+	virtual const std::string_view GetText() const final;
+
+	virtual void SetText(const std::string& text) final;
+};
+
+#endif