Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add to immutable trie returns original trie if no changes made #5

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

iand
Copy link
Contributor

@iand iand commented Oct 18, 2023

The TrieRT relies on this property to detect whether a node was added to the routing table or not. The bug doesn't cause any adverse effects to the TrieRT but makes it less efficient.

Comment on lines +218 to +225
for _, kk := range sampleKeySet.Keys {
next, err := Add(tr, kk, nil)
require.NoError(t, err)
// trie has not been changed
require.Same(t, tr, next)
}
require.Equal(t, len(sampleKeySet.Keys), tr.Size())

Copy link
Contributor

@dennis-tra dennis-tra Oct 18, 2023

Choose a reason for hiding this comment

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

Does it make sense to assert the counter-case as well? Like, if I added the same key with different data that the tries are NotSame? Not sure if this is covered by another test already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can add that test

@iand iand merged commit dbf0c23 into main Oct 18, 2023
4 checks passed
@iand iand deleted the fix-trie-add-immutable branch October 18, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants