add method for zero alloc byte slice to str

[ahrav]

Description:

Replace all detector byte slice to string operations with a zero alloc alternative.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[mcastorina]

Very cool! Do you foresee any dangers with this approach? Does it make it easier to accidentally modify the chunk data?

[rosecodym]

immutabilityProperty := func(b []byte) bool {
	if len(b) == 0 {
		return BytesToString(b) == string(b)
	}
	s := BytesToString(b)
	b[0] = 0 // modify byte slice
	return s == BytesToString(b)
}

Isn't this the opposite of immutability? If you create a string from a slice, and then you modify the slice, and then you create another string from the modified slice, the "new" string equals the "old" string. Since your slice modification wasn't a no-op, this means that the original string you created somehow changed. Right?

[ahrav]

immutabilityProperty := func(b []byte) bool {
	if len(b) == 0 {
		return BytesToString(b) == string(b)
	}
	s := BytesToString(b)
	b[0] = 0 // modify byte slice
	return s == BytesToString(b)
}

Isn't this the opposite of immutability? If you create a string from a slice, and then you modify the slice, and then you create another string from the modified slice, the "new" string equals the "old" string. Since your slice modification wasn't a no-op, this means that the original string you created somehow changed. Right?

Yea that was poorly named. It should've been something closer to TestBytesToString_SharedDataConsistency .

[ahrav]

Very cool! Do you foresee any dangers with this approach? Does it make it easier to accidentally modify the chunk data?

I don't think this would be significantly more dangerous than what we currently have. Given the memory savings it seems like a worthwhile tradeoff imo.

[rosecodym]

I am extremely wary of this. You've created a new, boring-looking function with an extremely non-obvious restriction on its use - namely, that the passed-in slice must not be mutated at all after the function returns. (This restriction is so non-obvious that you've even written a test that explicitly violates it. Other maintainers don't stand a chance!) And the consequences of breaking this restriction are unknowable and potentially catastrophic, because breaking it means breaking internal assumptions that the go runtime makes.

A function like that might be possible to get away with if it were only called from a small number of very tightly controlled places. But it's not - this function was created to be used in the part of the application that is largely community-written and -managed. This feels like passing out lighters during a fireworks factory tour.