search

ASU-cubesat / cfdp-rs

A rust implementation of the CCSDS File Delivery Protocol (CFDP)
MIT License
9 stars 2 forks source link

What about making the VariableID Copy? #1

Closed xpromache closed 1 year ago

xpromache commented 1 year ago

Hello,

I've been playing with your cfdp library and I think it could be a nice improvement if you made VariableID copy - that avoids doing clone all over the place.

I think it should be ok since it's a small structure (basically , don't you think?

mkolopanis commented 1 year ago

I think this is a good idea. Especially for only wrapping a primitive copy is probably reasonable. I probably initially left it off to make sure I was not moving something unexpectedly but overall I agree with you.

xpromache commented 1 year ago

Perfect, and I think there is another one - the file size which is also a enum(u32,u64).

xpromache commented 1 year ago

A more radical proposal: what about getting rid of FileSizeSensitive and replace it with a u64?

There is the information in the transaction if it's a small or large file and that information can be used in the few cases one needs to know to convert the u64 to u32. It will simplify the code quite a bit and also increase peformance because right now each time you deal with the file offset Rust has to check which version of the enum to use. Plus that u64 is half the size of the FileSizeSensitive enum (8 vs 16 bytes).

mkolopanis commented 1 year ago

That's an interesting idea. Especially if inside the transaction we already know when we need to convert down from a u64 to u32 this could simplify a lot.