zetzit / zz

πŸΊπŸ™ ZetZ a zymbolic verifier and tranzpiler to bare metal C
MIT License
1.6k stars 52 forks source link

introduce 'slice::slice::read{u,i}{8,16,32,64}{be,le}()' API #135

Closed jwerle closed 3 years ago

aep commented 3 years ago

makes sense to have, but not happy about the naming

read implies you can read multiple packed integers, but actually this is a conversion function and it ignores endianess.

i'd rather call it to_u8

aep commented 3 years ago

actually that tells me MutSlice is poorly named. while MutSlice is a writer with a write state, Slice does not have a read state

aep commented 3 years ago

oh wait, completely missed the offset. i guess you CAN read multiple packed integers that way, so the name is fine. just change the docs to say

/// read 8 bytes from offset as a u64 in host endianess

or something similar that mentions offset

and make sure the endianess is actually host endianness. I believe shift is defined endianness independant, so your code is wrong on one or the other? i'm not sure. shift and endianess confuses me alot.

jwerle commented 3 years ago

@aep Yeah I am rewriting to use the byteorder module and make to_u{8,16,32,64}{be,le}() variants. I can rename to_ back to read

aep commented 3 years ago

host endianess is fine, push8 does that too.

i'm just using memcpy in push because shift is so confusing. But if you can verify your code is always using host endianess, its fine

jwerle commented 3 years ago

@aep just pushed up some changes if you are free to take a look