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 Copy func #123

Merged
merged 7 commits into from
Oct 12, 2021
Merged

Add Copy func #123

merged 7 commits into from
Oct 12, 2021

Conversation

habuvo
Copy link
Contributor

@habuvo habuvo commented Oct 11, 2018

There is need to make independence copy of Decimal in any cases. I think it's better have special function for it due to clean code.

@mwoss
Copy link
Member

mwoss commented Mar 29, 2021

Sorry for answering so late (over 2 years XD)
I think this is a good addition to the library. Could you rebase with a master? I would love to merge this PR.

Copy link
Member

@mwoss mwoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :3

@@ -1553,7 +1553,7 @@ func (d Decimal) Sin() Decimal {
sign = !sign
j -= 4
}
z := d.Sub(y.Mul(PI4A)).Sub(y.Mul(PI4B)).Sub(y.Mul(PI4C)) // Extended precision modular arithmetic
z := d.Sub(y.Mul(PI4A)).Sub(y.Mul(PI4B)).Sub(y.Mul(PI4C)) // Extended precision modular arithmetic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you revert this line? If this is gofmt issue, I will address this in separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I trying to revert it right now

decimal.go Outdated
@@ -1684,3 +1683,15 @@ func (d Decimal) Tan() Decimal {
}
return y
}

// Copy makes instance of d with same value and different pointer.
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I think this line is unnecessary :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

@habuvo habuvo requested a review from mwoss March 30, 2021 14:20
@habuvo
Copy link
Contributor Author

habuvo commented Mar 30, 2021

I've resolved conflicts and now it is ready to merge

@habuvo
Copy link
Contributor Author

habuvo commented Mar 30, 2021

Or may be it will be better to create a new PR from the current master?

@mwoss
Copy link
Member

mwoss commented Mar 30, 2021

You can try to create a new PR if you have time. Thanks to that we would get rid of those format issues. :D

@Terminator637
Copy link

Any news on this?

@mwoss
Copy link
Member

mwoss commented Sep 29, 2021

I don't want to take credits from @habuvo for creating this PR, but still, linting problems are not resolved. So I not gonna merge it to the main branch.

@mwoss
Copy link
Member

mwoss commented Oct 12, 2021

I decided to merge this PR in the current state and fix all the issues I spotted afterwards, as I don't want to take any credits from @habuvo for this PR

@mwoss mwoss merged commit 013e52d into shopspring:master Oct 12, 2021
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.

3 participants