Skip to content

Add Clone() implementation for StringMap - #852

Merged
KyleSanderson merged 4 commits into
masterfrom
cloneitall
Jul 15, 2020
Merged

Add Clone() implementation for StringMap#852
KyleSanderson merged 4 commits into
masterfrom
cloneitall

Conversation

@Headline

Copy link
Copy Markdown
Member

We implemented a clone for ArrayList, but ArrayStack and StringMap were left without one. This pull request adds deep-copy cloning to said structures. Construct generally looks like

ArrayStack mystack = // something, lets assume it also has data
ArrayStack copy = mystack.Clone();

delete mystack;
delete copy;

Fixes #796

@asherkin

Copy link
Copy Markdown
Member

This pull request adds deep-copy cloning to said structures.

The implementation is a shallow copy (which I think is all we can sanely do and matches ArrayList), was something else intended?

@Headline

Headline commented Jul 18, 2018

Copy link
Copy Markdown
Member Author

@asherkin unless i'm mistaken, these are deep copies. Any modifications to one structure will leave the other completely unaffected. Trie stuff like Entry::setString allocs input string len+1 and strcopy's the data, for example.

CCellArray::clone() also looks like a deep copy

ICellArray *clone()
{
CellArray *array = new CellArray(m_BlockSize);
array->m_AllocSize = m_AllocSize;
array->m_Size = m_Size;
array->m_Data = (cell_t *)malloc(sizeof(cell_t) * m_BlockSize * m_AllocSize);
if (!array->m_Data)
{
delete array;
return NULL;
}
memcpy(array->m_Data, m_Data, sizeof(cell_t) * m_BlockSize * m_Size);
return array;
}

@Headline
Headline requested a review from KyleSanderson July 19, 2018 01:43

@KyleSanderson KyleSanderson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this actively desired? It seems like a thing that could be used but giving some lineage on the drive behind a PR helps better understand intentions.

Comment thread plugins/include/adt_stack.inc
Comment thread core/logic/smn_adt_trie.cpp Outdated
Comment thread core/logic/smn_adt_trie.cpp Outdated
Comment thread core/logic/smn_adt_trie.cpp
Comment thread plugins/include/adt_trie.inc Outdated
Comment thread public/sm_trie_tpl.h
@Headline

Headline commented Jul 19, 2018

Copy link
Copy Markdown
Member Author

@KyleSanderson I’m unsure if ArrayStack’s clone is used (or if people use ArrayStack at all), but I think being able to clone a StringMap is a really good addition into core. I can’t double check, but I don’t think it’s possible to distinguish a key’s mapped type so this couldn’t even be done native in sp, so a clone couldn’t be accomplished prior to this pr nevermind getters fail.

@KyleSanderson

Copy link
Copy Markdown
Member

Stack is used by many; cloning a stack I'm not sure on as it likely predates most of us. The trick is to jump on BZ (or now here) and looking at why something was added. Nice-to-haves are legit, just wanted some lineage.

All good.

@Headline

Copy link
Copy Markdown
Member Author

Ah, my bad. All should be well now.

@Headline

Copy link
Copy Markdown
Member Author

For ref/lineage: simple tests seem to pass as-expected.

public void OnPluginStart()
{
	StringMap original = new StringMap();
	original.SetValue("bool", true);
	original.SetValue("handle", new ArrayList());
	original.SetString("string", "hello world");
	
	StringMap copy = original.Clone();
	delete original;
	
	bool ret = false;
	bool value;
	ret = copy.GetValue("bool", value);
	PrintToServer("bool test: %s", (ret && value)?"successful":"unsuccessful");
	
	ArrayList handle;
	ret = copy.GetValue("handle", handle);
	PrintToServer("hndl test: %s", (ret && handle != null)?"successful":"unsuccessful");

	char string[32];
	ret = copy.GetString("string", string, sizeof string);
	PrintToServer("string test: %s", (ret && strlen(string) > 0)?"successful":"unsuccessful");
	
	delete copy;
	delete handle;
}

@KyleSanderson KyleSanderson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's it, mention the PR # like every other line item cause and you're probably golden.

Comment thread core/logic/smn_adt_trie.cpp Outdated
@KyleSanderson
KyleSanderson dismissed their stale review July 30, 2018 04:57

Passing onto a peer.

@Headline Headline added the Feature Request user requested feature label Aug 19, 2018
@peace-maker

Copy link
Copy Markdown
Member

@Headline now that #1304 was merged, this could be reduced to only adding a StringMap.Clone()?

@Headline
Headline requested a review from KyleSanderson July 11, 2020 00:28
@Headline Headline changed the title Add Clone() construct for more structures Add Clone() implementation for StringMap Jul 11, 2020
@KyleSanderson
KyleSanderson merged commit 5fa25e7 into master Jul 15, 2020
@KyleSanderson
KyleSanderson deleted the cloneitall branch July 15, 2020 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Request user requested feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: CloneTrie() and CloneStack()

5 participants