MathNya / umya-spreadsheet

A pure rust library for reading and writing spreadsheet files
MIT License
284 stars 46 forks source link

Suggestion: Consider not using references for all numeric function parameters #209

Open fgimian opened 3 months ago

fgimian commented 3 months ago

Hey again,

I noticed that various functions take &u32s as arguments:

e.g.

pub fn insert_new_row(&mut self, row_index: &u32, num_rows: &u32)

This is actually likely less efficient and also less ergonomic than just taking in u32s because as it stands today, the pointer to the u32 would need to be copied instead (which is typically 64-bits / 8 bytes) while creating a copy of the number itself would only take 32-bits / 4 bytes.

I think this is why you will see that the Copy trait is implemented for such primitive types in Rust, as it is often as efficient or more efficient to just taking a copy of the value than a copy of the pointer reference.

Just a thought, not sure if I'm missing the reason why references were used for these cases.

Cheers Fotis

Expurple commented 3 months ago

Yeah, this always seemed weird and unergonomic to me.

Also, get_ prefixes are uncommon in Rust, they only add noise IMO.

schungx commented 3 months ago

I have since gotten used to it since changing this is likely to be a HUGE code-breaking exercise...

MathNya commented 3 months ago

Thank you for pointing this out. I did not understand the specifications regarding copying.

However, I would have to raise a major version to change this. We just recently went to 2.0.0 and will wait a while before considering a response.

MathNya commented 3 months ago

Also, get_ prefixes are uncommon in Rust, they only add noise IMO.

I did not know this either. I usually use PHP so get_ was a given.

I will consider this too when I do a major version in a while.

schungx commented 3 months ago

Also, get_ prefixes are uncommon in Rust, they only add noise IMO.

I did not know this either. I usually use PHP so get_ was a given.

I will consider this too when I do a major version in a while.

Honestly, it isn't idiomatic Rust style, but I'm OK with it. Many languages have this syntax, like JavaScript's obj.getMyTeaCup(), C#'s obj.GetMyTeaCup() or obj.MyTeaCup and Rust sometimes has obj.get_my_tea_cup() instead of obj.my_tea_cup()...

But in Rust, usually the modifier get_ is used when some work needs to be done in order to return some info, while the syntax without get_ is used when it is a trivial reference, like fn my_tea_cup(&self) -> &TeaCup { self.my_tea_cup }... which is usually coupled with #[inline(always)] to note that fact.

I think the reference parameters like &i32 are way more annoying that those get_'s...