tchap / go-patricia

A generic patricia trie (also called radix tree) implemented in Go (Golang)
MIT License
276 stars 58 forks source link

put function should copy Prefix #10

Open emrekzd opened 10 years ago

emrekzd commented 10 years ago

Prefix is a named type of []byte, so passing it to put function (called by Set and Insert) passes a reference to the slice value. put should make a copy of the key:

    t := patricia.NewTrie()

    key := patricia.Prefix("foo")
    t.Insert(key, true)

    // alter the key slice
    key[0] = 'b'

    fmt.Println("boo key: ", t.Get(patricia.Prefix("boo")))
    fmt.Println("foo key: ", t.Get(patricia.Prefix("foo")))

Output:

    boo key:  true
    foo key:  <nil>

Sending a pull request for a fix.

tchap commented 10 years ago

I am not sure we need to copy the prefix by default, perhaps it would be better to leave it like that and just document that the tree is taking over the ownership of the prefix that is passed there? @emrekzd

emrekzd commented 10 years ago

hello Ondrej. It is up to you but I don't agree with not copying it.

The issue is type casting a string to []byte causes the argument to be passed as a slice variable and gives the caller a way to modify trie keys without using the API. This becomes an issue when you pass a Prefix variable as argument.

It is more easy to see this if you think of the Visitor function func(p Prefix, i Item) error where Prefix is passed an argument. Because you get a reference to the prefix modifying it / inserting it into another trie will result in unexpected and counterintuitive behavior.

In fact it would be more convenient and safe to design the API to expect string types and have them converted to []byte internally, but since you want to keep the API as it is I think you should be making copies of the keys.

tchap commented 10 years ago

@emrekzd Alright, I will think about it...