Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
AaronKutch opened this issue May 17, 2018 · 12 comments
Open

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

AaronKutch opened this issue May 17, 2018 · 12 comments

Comments

@AaronKutch
Copy link
Contributor

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
Copy link
Owner

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

@AaronKutch
Copy link
Contributor Author

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

@Robbepop
Copy link
Owner

Robbepop commented May 18, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

AaronKutch commented Sep 12, 2018

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Robbepop
Copy link
Owner

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants