Skip to content

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Jul 26, 2022

No description provided.

@rotu rotu mentioned this pull request Jul 26, 2022
@konsumlamm
Copy link
Contributor

IMO this is a really bad idea. Not only do I think that implicit conversions in general are a bad idea (the conversion may happen at any point and you have to remember that it may happen), but this also introduces a hidden allocation.

@rotu
Copy link
Contributor Author

rotu commented Jul 26, 2022

IMO this is a really bad idea. Not only do I think that implicit conversions in general are a bad idea (the conversion may happen at any point and you have to remember that it may happen), but this also introduces a hidden allocation.

Very fair. I think implicit conversions are a bad idea in general, too. But as a general rule, Nim allows implicit widening conversions. Any objections to implicit widening for binary arithmetical operations?

@konsumlamm
Copy link
Contributor

Any objections to implicit widening for binary arithmetical operations?

If you mean operations that take BigInt and int, no, not from me (although there isn't really any implicit conversion then), but they should use an optimized implementation (i.e. not just convert the int to a BigInt), e.g. additionInt, subtractionInt.

@rotu
Copy link
Contributor Author

rotu commented Jul 26, 2022

@konsumlamm
What's the overhead actually look like for a BigInt when a normal int would suffice?
Is the backend compiler really not smart enough to optimize out a BigInt operand?

@konsumlamm
Copy link
Contributor

Converting the int to a BigInt would allocate a new seq, which can be avoided. I highly doubt this is optimized away.

@rotu rotu closed this Jul 27, 2022
@rotu
Copy link
Contributor Author

rotu commented Jul 27, 2022

Converting the int to a BigInt would allocate a new seq, which can be avoided. I highly doubt this is optimized away.

I'm no master at reverse engineering, but I think I confirmed that the allocation is not optimized away on my machine. Still no idea of the performance impact though.

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