search

qmuntal / gltf

Go library for encoding glTF 2.0 files
https://www.khronos.org/gltf/
BSD 2-Clause "Simplified" License
241 stars 32 forks source link

Remove usage of unsafe when checking header size #55

Closed mokiat closed 2 years ago

mokiat commented 2 years ago

This PR has the decoder use binary.Size in favour of int(unsafe.Sizeof(header)), since the binary package is either way used for the parsing of the header.

This makes the implementation a bit more robust, since it should handle architectures that might align structures differently. Though in this particular case, due to the simplicity of the struct, it might actually be a bit more of a nit picking.

In addition, it also fixes a small typo (I assumed too small for a separate PR).

The only other place using the unsafe package that remains is the binary/unsafe.go file. I might be up to writing a separate PR in the future that reworks that to use the more manual approach, as used in binary/binary.go, if the owner of this repo is open to the idea of getting rid of unsafe.

qmuntal commented 2 years ago

Thanks for this contribution! The less unsafe the better.

Do you have a particular need to use this package in an architecture which disallow unsafe?

mokiat commented 2 years ago

Thanks for this contribution! The less unsafe the better.

Do you have a particular need to use this package in an architecture which disallow unsafe?

No, not particularly. I will be using it to convert models from gltf to my internal format for a game framework/engine I am working on as a hobby project. Up until today I was using something I had quickly put together on my own in the past but I saw this library today and decided to switch.

While checking if it supports glb and if it would work in my scenario, I noticed the unnecessary usage of unsafe so decided to create this PR. If you feel this is too much of a nit picking and prefer to keep the library as is, I won't mind if you close the PR.

qmuntal commented 2 years ago

If you feel this is too much of a nit picking and prefer to keep the library as is, I won't mind if you close the PR.

I'll merge this PR, I don't think it affects performance and itis more correct.

About the usage of unsafe in unsafe.go, I'm open to remove it if performance and maintainability is not affected much.