search

fe-lang / sonatina

Apache License 2.0
47 stars 6 forks source link

Implement `cranelift_entity::ReservedValue` for `Type` #64

Closed emhane closed 2 months ago

emhane commented 2 months ago

Implements cranelift_entity::ReservedValue for Type, to use as PackedOption

emhane commented 2 months ago

*type is only 8 bytes

Y-Nak commented 2 months ago

oops, yeah. I misunderstood something, sorry.

Y-Nak commented 2 months ago

Then, it'd be reasonable to implement ReservedValue for type.

emhane commented 2 months ago

thought so, based on the explanation. thanks for the explanation.

Y-Nak commented 2 months ago

Hmm, but I still think using Option<Type> could be slightly better. The reason is that Rust applies null pointer optimization, meaning that if Type does not utilize all possible bit patterns, the size of Option<Type> can remain 8 bytes, just like Type. From a performance perspective, just using Option<Type> would be slightly better. However, the performance difference is likely very minor, especially if Option<Type>/PackedOption<Type> is not used frequently. So it'd be fine as is, I think.

emhane commented 2 months ago

Hmm, but I still think using Option<Type> could be slightly better. The reason is that Rust applies null pointer optimization, meaning that if Type does not utilize all possible bit patterns, the size of Option<Type> can remain 8 bytes, just like Type. From a performance perspective, just using Option<Type> would be slightly better. However, the performance difference is likely very minor, especially if Option<Type>/PackedOption<Type> is not used frequently. So it'd be fine as is, I think.

ah, I see, because variant Type::Compound(u32), actually only needs 5 bytes. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=803c11e6ccc6789c7ffa2291e53e43f9.

we probably should revert this pr.