search

getty-zig / getty

A (de)serialization framework for Zig
https://getty.so
MIT License
183 stars 14 forks source link

de: Remove commented-out defer #126

Closed ibokuri closed 1 year ago

ibokuri commented 1 year ago

@polykernel can I delete this comment?

polykernel commented 1 year ago

Yeah definitely, the line is unnecessary given BoundedEnumMultiSet can only stores enums.

background (if anyone is interested) I originally added the line in because values inserted into a `BoundedEnumMultiSet` are only needed temporarily for the Indexer to translate them to indices into the backing `EnumArray`. If the values were allocated (e.g. string), then they must be freed after each iteration otherwise a leak would manifest as `BoundedEnumMultiSet` doesn't hold onto them. I later noticed the defer would unconditionally force the allocator to be non-null even if no allocation is done which is not ideal. I commented it out while working on adding something akin to `isKeyAllocated` to the SeqAccess interface in order to avoid this drawback. When I finished the implementation, I realized I should have discuss this idea first in GitHub issues before adding it into the PR and thus reverted the changes. Alas, I forgot to remove this line.
ibokuri commented 1 year ago

So right now, keys aren't freed at all for enum multisets (in visitSeq), but you have a incoming proposal for an isElementAllocated method (or something like that which would let us free them properly? Is that correct?

polykernel commented 1 year ago

Yeah, keys (values?) for BoundedEnumMultiset do not need to be freed because the enums returned by visitEnum as implemented in the repo are stack values. Although this doesn't stop a contrived implementation of visitEnum from returned a heap-allocated enum. The current correct solution without isElementAllocated and friends is to always free and force unwrapping the allocator, but this has the (non-critical) drawback I mentioned above. I have filed an proposal regarding isElementAllocated and isValueAllocated here: https://github.com/getty-zig/getty/issues/127