Migrate CDataPack from a Compact Cassette tape. #688

KyleSanderson · 2017-09-19T23:27:04Z

This lift and shifts CDataPack from a linear datatype to something using a vector with type mixed in. Iterators will change (again), so plugins manually using SetPackPosition without ReadPackPosition (against the API) will experience breakage; if this even builds.

Headline · 2017-09-20T01:52:57Z

Floats seem to show 0.0 on ReadFloat.

public void OnPluginStart()
{
	int integer = 32;
	float floatingPoint = 32.0;
	char[] string = "Hello.";
	
	DataPack pack = new DataPack();
	
	pack.WriteCell(integer);
	pack.WriteFloat(floatingPoint);
	pack.WriteString(string);
	
	DataPackPos pos = pack.Position;
	
	pack.Reset();
	
	int integer2;
	float floatingPoint2;
	char string2[8];
	
	integer2 = pack.ReadCell();
	floatingPoint2 = pack.ReadFloat();
	pack.ReadString(string2, sizeof(string2));
	
	PrintToServer("int: %i, float: %.2f, string: %s, pos: %i",
					integer2,
					floatingPoint2,
					string2,
					pos);
	
	

	delete pack;
}

Output:

int: 32, float: 0.00, string: Hello., pos: 1
[SM] Plugin testcase.smx reloaded successfully.


        Migrate CDataPack from a Cassette Tape.

Tested-By: Headline22.


        Remove last IsReadable param pass.


        populate len still if CDataPack::ReadString is unreadable or the wron…

…g type.

Headline · 2017-09-20T05:51:19Z

So the test case for DataPack operations seems to work as expected now. Added errors for invalid type works great too.

👍 From me

Test case

public void OnPluginStart()
{
	int integer = 32;
	float floatingPoint = 32.0;
	char[] string = "Hello.";
	
	DataPack pack = new DataPack();
	
	pack.WriteCell(integer);
	pack.WriteFloat(floatingPoint);
	pack.WriteString(string);
		
	pack.Reset();
	
	int integer2;
	float floatingPoint2;
	char string2[8];
	
	integer2 = pack.ReadCell();
	floatingPoint2 = pack.ReadFloat();
	
	pack.ReadString(string2, sizeof(string2));
	
	PrintToServer("int: %i, float: %.2f, string: %s",
					integer2,
					floatingPoint2,
					string2);
	
	char string3[8];
	PrintToServer("%i", pack.Position);
	pack.Position--; // go back to string
	PrintToServer("%i", pack.Position);

	pack.ReadString(string3, sizeof(string3));
	PrintToServer("%s", string3);
	
	delete pack;
}

Actual output

int: 32, float: 32.00, string: Hello.
3
2
Hello.
[SM] Plugin testcase.smx reloaded successfully.

Fyren

In theory, I think we want an IDataPack2. IsReadable's semantics basically assume the one contiguous block of memory model and this has pretty different performance characteristics.

In practice, do we care? Does any extension even use this?

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.h

+	typedef union {
+		cell_t cval;
+		float fval;
+		void *vval;


This should definitely not be a void*, as mentioned in IRC. An AString* and a uint8_t* if raw is kept, otherwise just the AString*. This allows you to get rid of the casts.

Fyren · 2017-10-03T14:52:33Z

core/logic/smn_datapacks.cpp

 	{
 		return pContext->ThrowNativeError("DataPack operation is out of bounds.");
 	}

+	if (pDataPack->GetCurrentType() != CDataPackType::Cell)
+	{
+		return pContext->ThrowNativeError("Invalid Datapack Type (got %d / expected %d).", pDataPack->GetCurrentType(), CDataPackType::Cell);


Please check all the error messages for consistency, including the ones you didn't touch. The docs use "data pack" and errors shouldn't be in title case but normal sentence case.

I'd also recommend prettifying the enum types for the user so they see names and not the integer value of the enum. Lots (most? all?) of our other errors don't do anything like that, so it's definitely not required.

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

 }

 void CDataPack::ResetSize()
 {
-	m_size = 0;
+	this->Initialize();
 }

 size_t CDataPack::CreateMemory(size_t size, void **addr)


Do we even know if a single person is using the generic/raw memory functions? This functionality is not exposed to plugins, so it'd have to be an extension. I'd be in favor of axing these completely. (Since they're in the interface, just make them no-ops/return null.)

If they're kept, the docs in IDataPack.h about pointers being invalidated (in the case of realloc) are wrong and should be updated.

You also alloc a byte more than necessary? Did you mean to store the size here (originally stored as 32-bits)? Related to the behavior change I mention for ReadMemory?

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

-
-	size_t real_len = *(size_t *)m_curptr;
+	if (!this->IsReadable())
+		return "";


The API says "pointer to the string, or NULL if out of bounds," so the old behavior seems correct to me. We should actually use nullptr now, though (and switch any other uses in the file as well).

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

-		return NULL;
-	}
+	if (this->elements[this->position].type != CDataPackType::String)
+		return "";


The API doesn't say what would happen in this case, but the old behavior was to return NULL, so I'd also make it nullptr.

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

+	if (this->elements[this->position].type != CDataPackType::Raw)
+		return nullptr;
+
+	return this->elements[this->position].pData.vval;
 }

 void *CDataPack::ReadMemory(size_t *size) const


Behavior here appears to have changed? The old behavior checked the stored size was <= the requested size and returned null if it wasn't. It also returned the real size of the memory chunk, which you haven't stored.

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

-	m_pBase = (char *)malloc(DATAPACK_INITIAL_SIZE);
-	m_capacity = DATAPACK_INITIAL_SIZE;
-	Initialize();
+	this->Initialize();


Style: Why are you using this-> everywhere? SM code in general doesn't appear to use this style.

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

-void CDataPack::CheckSize(size_t typesize)
-{
-	if (m_curptr - m_pBase + typesize <= m_capacity)
+	for (size_t iter = 0; iter < this->elements.length(); ++iter)


Style: this isn't an iterator, it's an index.

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

-	m_curptr += real_len + 1;
-
-	return str;
+	return val.chars();
 }

 void *CDataPack::GetMemory() const


This is not the same behavior. The analogue in the new implementation would be returning the address of the InternalPack. However, I don't see how/why anyone would ever use this function because the pointer is opaque to the user.

Fyren · 2017-10-03T14:52:33Z

core/logic/CDataPack.cpp

-	m_curptr += sizeof(float);
-	return val;
+
+	return this->elements[this->position++].pData.fval;
 }

 bool CDataPack::IsReadable(size_t bytes) const


The docs here utterly suck. The imply the old implementation and so this is not really satisfiable with the new one.


        Fyren Fixes(TM)(R)(C).


        Deprecate IDataPack.

KyleSanderson force-pushed the tapedeck branch from a507ab7 to 1972508 Sep 19, 2017

KyleSanderson requested a review from Fyren Sep 20, 2017

Migrate CDataPack from a Cassette Tape.

Loading status checks…

35420d6

Tested-By: Headline22.

KyleSanderson force-pushed the tapedeck branch from 5570cda to 35420d6 Sep 20, 2017

KyleSanderson added 2 commits Sep 20, 2017

Remove last IsReadable param pass.

Loading status checks…

5524ed8

populate len still if CDataPack::ReadString is unreadable or the wron…

Loading status checks…

5dbabb3

…g type.

KyleSanderson requested a review from asherkin Sep 20, 2017

Fyren requested changes Oct 3, 2017

View changes

KyleSanderson added 2 commits Oct 3, 2017

Fyren Fixes(TM)(R)(C).

Loading status checks…

74cf043

Deprecate IDataPack.

Loading status checks…

6a16ddc

KyleSanderson force-pushed the tapedeck branch from d076236 to 6a16ddc Oct 3, 2017

KyleSanderson merged commit 5f5a6b3 into master Dec 30, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

CrazyHackGUT referenced this pull request Jan 19, 2018

SM 1.10 #22

Closed

asherkin deleted the tapedeck branch Feb 2, 2018

This was referenced Jul 9, 2018

Match old DataPack behavior when overwriting data Headline/sourcemod#15

Closed

Match old DataPack behavior when overwriting data #848

Merged

KyleSanderson referenced this pull request Jul 16, 2018

Fix memory leak regression from #848. #850

Merged

alliedmodders/sourcemod

Migrate CDataPack from a Compact Cassette tape. #688

Migrate CDataPack from a Compact Cassette tape. #688

KyleSanderson commented Sep 19, 2017

This comment has been minimized.

Headline commented Sep 20, 2017

This comment has been minimized.

Headline commented Sep 20, 2017 •

edited

Fyren left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Sponsor alliedmodders/sourcemod

alliedmodders/sourcemod

Join GitHub today

Migrate CDataPack from a Compact Cassette tape. #688

Conversation

KyleSanderson commented Sep 19, 2017

This comment has been minimized.

Headline commented Sep 20, 2017

This comment has been minimized.

Headline commented Sep 20, 2017 • edited

Fyren left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Headline commented Sep 20, 2017 •

edited