Enet4 / nifti-rs

Rust implementation of the NIfTI-1 format
Apache License 2.0
40 stars 11 forks source link

Improve integration with code type definitions #18

Closed Enet4 closed 5 years ago

Enet4 commented 5 years ago

At first I considered modifying the attributes of NiftiHeader to contain these types immediately upon reading the header. With the repr directives, the header could still be packed without wasting memory and providing an extra layer of abstraction. However, I feel reluctant to make the entire reading process break just because of a single unknown code, which for the most part (datatype is an exception) are not critical. By keeping unvalidated codes, the library will give more flexibility to eventual file diagnostic tools.

nilgoyette commented 5 years ago

take self by value (cheaper than by ref)

Just to be sure, does calling a method on it consume the value? Or it's safe because it's a u8/u16 and they are defined as Copy objects? Probably the later.

Good job, lgtm.

Enet4 commented 5 years ago

Just to be sure, does calling a method on it consume the value? Or it's safe because it's a u8/u16 and they are defined as Copy objects? Probably the later.

Yes, it is actually preferable for small Copy types to have consuming methods. It is perceived by the compiler as consuming a copy of the value, which is cheaper than making a reference to it (u8 vs &u8). In practice, usage doesn't change, but it's more idiomatic and leaves a bit more room for machine code optimization [citation_needed]. Primitive integer types in stdlib also follow this guideline (e.g. i16), and Clippy has a lint for this.