Robbepop / apint

Arbitrary precision integers library.
Other
26 stars 4 forks source link

The `From<[u64;2]>` functions use big endian #20

Open AaronKutch opened 6 years ago

AaronKutch commented 6 years ago

I was writing unit tests when I found this out. I always use little endian (except when typing out literals), but if you are sure about this be sure to document it. In fact, there could be a section just about endianness.

Robbepop commented 6 years ago

You are totally right that this should be documented more thoroughly. During development I slipped too many times over this myself already ...

AaronKutch commented 6 years ago

So you are going to keep it as big endian? I am curious about what the reason is.

Robbepop commented 6 years ago

I am not sure if I want to keep it. I did that on purpose to help users that mentally wrap numbers in their head in bigendian (which I do myself for example). So if you really don't like it we can change it. But please let's do this all in another pull request.

AaronKutch commented 6 years ago

After I submit my large pull request, can you convert the From functions to little endian? I have some code that can flip all the numbers around but I ran into some complex errors that I think you could solve much faster than me.

AaronKutch commented 6 years ago

Specifically, I could do a pull request that has switched all the ApInt::from([a,b,c,d]) to ApInt::from([d,c,b,a]) and you could do the rest. There's a .rev() in the impl From<[u64; $n]> for ApInt that I presume makes it big endian, but when I remove it there's a big mess of errors from different parts of the library I'm not familiar with

Robbepop commented 6 years ago

Hey Aaron, sorry for my slow response. I am not sure if I want to process such a possibly big refactoring at the moment since I am also not into the code base right now. Since this changes interfaces it might touch the entire code (and breaks it), especially the many unit tests. So I think we make this critical change on another time for now.

AaronKutch commented 6 years ago

My stance is that a 0.3.0 release of apint after I finish the arithmetic rewrite will force users of this crate to rewrite a lot anyway and consider replacing certain things with new features. I've been letting the scope creep get in my way of finishing the arithmetic function rewrite and I will make sure that we have a clean commit history to work with before we start a little endian rewrite.

Robbepop commented 6 years ago

I thought about this. We should keep the From implementation at big endian since that's what users normally would expect (I assume). However, we should really really document such behaviour better. I for myself find endianess issues always very confusing.

AaronKutch commented 6 years ago

I have gotten used to the little endian output of debug printed ApInts. I very strongly prefer little endian for any abstraction higher than a straight string of human readable numbers. Big endian also has performance implications since little endian is more natural for computers and programming alike. There would be a .rev inside the functions and .rev() that I use outside (in fact when I remove the from_vec_u64 function in favor of another I will have to put .rev() on stuff).

AaronKutch commented 6 years ago

I say we do it and change the endianness as part of 0.3

Robbepop commented 6 years ago

Okay for the sake of performance we maybe should do this. Or: We deprecate this API completely and replace it by something more explicit, like Apint::from_be (big-endian) and Apint::from_le (little-endian) and Apint::from_ne (native-endian: always most performance locally).

AaronKutch commented 6 years ago

deprecation and adding ApInt::from_le and ApInt::from_be sounds excellent.