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

Add signum function #108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add signum function #108

wants to merge 1 commit into from

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Jul 25, 2022

No description provided.

@konsumlamm
Copy link
Contributor

Should this really allocate a new BigInt? I feel like this should just return an int, so that the user can choose if they want that extra allocation, by manually calling initBigInt. Do you have a concrete use casein mind for this, where it's more useful to have this return a BigInt?

@rotu
Copy link
Contributor Author

rotu commented Jul 26, 2022

Should this really allocate a new BigInt? I feel like this should just return an int, so that the user can choose if they want that extra allocation, by manually calling initBigInt. Do you have a concrete use casein mind for this, where it's more useful to have this return a BigInt?

The reason it returns a BigInt is for the invariant x == abs(x) * sgn(x). I'd be okay with returning an int if BigInt arithmetic allowed implicit widening (like other numeric types do).

@konsumlamm
Copy link
Contributor

I think we should support arithmetic where one of the arguments is an int sooner or later.

@rotu
Copy link
Contributor Author

rotu commented Jul 26, 2022

I think we should support arithmetic where one of the arguments is an int sooner or later.

Any reason why initBigInt(SomeSignedInt): BigInt isn't an implicit conversion? That seems like the easy solution

@rotu
Copy link
Contributor Author

rotu commented Jul 26, 2022

NVM - opened #109
Feel free to tell me there why this is a bad idea :-p

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

Successfully merging this pull request may close these issues.

2 participants