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

math/big: add Int.AddInt64, Int.CmpInt64 #29951

Open
TuomLarsen opened this issue Jan 26, 2019 · 11 comments

Comments

Projects
None yet
9 participants
@TuomLarsen
Copy link

commented Jan 26, 2019

Please consider adding big.Int methods Inc and Dec which would increase or decrease the given Int by one. It would be similar to the following code, except for the allocation:

x.Add(x, big.NewInt(1))

The motivation is that it is quite common operation and the code using it would be simpler and saving one allocation.

Alternatively, please consider AddInt(64) and SubInt(64), which would still save one allocation when knowing the increment/decrement fits a (64-bit) machine word.

Cursory look at Go source shows that the first alternative could be useful here and here, and the second one here.

Yet another alternative would be to expose intOne from int.go.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

You can declare some package variables called one, two, etc. and use them throughout.

x.Add(x, one)

This also lets you do things like

x.Cmp(one)

@odeke-em odeke-em changed the title math/big: Int.Inc, Int.Dec proposal: math/big: add Int.Inc, Int.Dec methods Jan 27, 2019

@gopherbot gopherbot added this to the Proposal milestone Jan 27, 2019

@gopherbot gopherbot added the Proposal label Jan 27, 2019

@ericlagergren

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

@TuomLarsen

This comment has been minimized.

Copy link
Author

commented Jan 30, 2019

@robpike Yes, package variables are one way of how to approach (although each package would have to possibly allocate the same big.NewInt(1)) this but the proposal is more about avoid allocations and simplifying the notation for arguments which fit machine word. Kind of like GMP _si and ui methods, of which I find that +1/-1 are the most useful.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Int.AddInt64 seems strictly better than Inc/Dec and matches the Int64 and SetInt64 methods. (Probably don't need SubInt64 if we have a general AddInt64. Also probably don't need AddUint64, SubUint64.)

But what else would it require adding? And? AndNot? Cmp? Div? DivMod? Mod? Mul? Or? Quo? QuoRem? Rem? Sub after all? Xor?

Just declaring a few globals with the constants you need seems to be the right answer most of the time. It's not clear that doubling the API surface is a significant enough win.

@TuomLarsen

This comment has been minimized.

Copy link
Author

commented Jan 31, 2019

Perhaps And, AndNot, Xor are not really necessary, Cmp can be worked around with Sign, IsInt64, Int64. So if I may summarize this gives 4 options:

  • Add every arithmetic operator (Add, Sub, Mul, QuoRem, Quo, Rem, DivMod, Div, Mod) and maybe Exp, each for Int64 and Uint64. This would be quite complete (akin to GMP) but it seems too specialized for stdlib to compensate for so many new methods.

  • Add only Add and Sub, for both Int64 and Uint64. I would argue Sub is needed even for Int64 case as the main motivation is to simplify code and to manually check whether an argument is negative or not to suitably change the sign so that it works for Add seems way too much work. And big.Int also has Sub even when it is not strictly necessary. Four methods, and really only two of which need a proper implementation as Int64 may be using Uin64 internally. The drawback is that it somehow "promises" there are more machine word methods like this, as shown by your question.

  • And only Inc and Dec. No need to worry about Uint, Int64, it does not suggest other "missing" machine word methods. Albeit more limited that any option above, perhaps it's still better than nothing.

  • Don't add anything. Clearly this is the easiest option but I believe that the above options have the potential to be generally useful.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Talked to @griesemer, who is inclined to start with just AddInt64 and CmpInt64 and wait for more compelling needs for any of the others. Those are clearly the most common.

@rsc rsc changed the title proposal: math/big: add Int.Inc, Int.Dec methods math/big: add Int.AddInt64, Int.CmpInt64 Feb 6, 2019

@rsc rsc modified the milestones: Proposal, Go1.13 Feb 6, 2019

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

I would start with none of them. If you add one, you'll end up adding them all, or at best dealing with a string of proposals to add them piecemeal. Please hold the line.

@TuomLarsen

This comment has been minimized.

Copy link
Author

commented Feb 12, 2019

@rsc If I understood correctly, AddInt64 would need to distinguish positive and negative values as the internal implementation of big.Int uses big.nat, i.e. to add a negative number it subtracts its absolute value. If there would indeed be such mechanism, why not expose it, i.e. to add both AddUint64 and SubUint64? (With or without AddInt64?) It is similar to Int.SetInt64/Int.SetUint64, Rat.SetInt64/Rat.SetUint64, I think unsigned 64-bits are useful.

@bmkessler

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

I don't understand the need for adding these methods to the standard library. As @robpike mentioned, package level variables already handle the proposed use case very well and the big.Int API is already very large.

@mazarin1

This comment has been minimized.

Copy link

commented Mar 4, 2019

Since AddInt64(x) would need to distinguish the sign and conver x to nat we can’t save much on allocations. And may be even more harmful for memory to do so in case of making increment functions. Only convenience I see is symantics. But I think std libs should follow: KISS.
Makes it easy to test, debug and understand

Solution like

  one := big.NewInt(1)
  counter := big.NewInt(0)
   ...
   counter.Add(counter,one)
   ...

are way better approach to issue.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I shall punt to Go1.14 as the jury is still hung on whether to implement it or not, but also there hasn't been movement.

@odeke-em odeke-em modified the milestones: Go1.13, Go1.14 Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.