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 isqrt_checked and standardize error format #18939

Merged
merged 3 commits into from Jul 22, 2023

Conversation

phoreverpheebs
Copy link
Contributor

@phoreverpheebs phoreverpheebs commented Jul 21, 2023

This PR changes isqrt to return a Result type instead of a runtime panic on negative input. This effectively eliminates all removable runtime panics in math.big (excluding the documented panics for division methods).

This PR also "standardizes" math.big error messages to conform to the format "module: error message", as well as fixing up most panics in *_test.v files to report the function's returned error message.

🤖 Generated by Copilot at 3e8250f

This pull request enhances the error handling and messages in the math.big module and its tests. It also refactors the random_number function to a common file for better code organization.

🤖 Generated by Copilot at 3e8250f

  • Return an error instead of panicking for negative inputs in isqrt function (link)
  • Add error handling to isqrt function calls in test_isqrt and isqrt_test_data (link, link)
  • Move random_number helper function to big_test.v file and use actual error variable in panic calls (link, link, link, link)
  • Prefix error messages with module name math.big in integer_from_radix function (link, link)

@medvednikov
Copy link
Member

medvednikov commented Jul 21, 2023 via email

@phoreverpheebs
Copy link
Contributor Author

func (Float) Sqrt ¶ added in go1.10 func (z Float) Sqrt(x Float) Float Sqrt sets z to the rounded square root of x, and returns it.If z's precision is 0, it is changed to x's precision before the operation. Rounding is performed according to z's precision and rounding mode, but z's accuracy is not computed. Specifically, the result of z.Acc() is undefined.The function panics if z < 0. The value of z is undefined in that case. I think a panic is also fine here, like in Go.

On 21 Jul 2023, at 23:04, phoebe @.
> wrote: This PR changes isqrt to return a Result type instead of a runtime panic on negative input. This effectively eliminates all removable runtime panics in math.big (excluding the documented panics for division methods). This PR also "standardizes" math.big error messages to conform to the format "module: error message", as well as fixing up most panics in _test.v files to report the function's returned error message. copilot:summary copilot:walkthrough You can view, comment on, or merge this pull request online at: #18939 Commit Summary • 3e8250f update File Changes (4 files) • M vlib/math/big/big_test.v (6) • M vlib/math/big/division_array_ops_test.v (2) • M vlib/math/big/integer.v (11) • M vlib/math/big/special_array_ops_test.v (16) Patch Links: • https://github.com/vlang/v/pull/18939.patchhttps://github.com/vlang/v/pull/18939.diff — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: @.
*>

Alright I'll just make it a PR to standardize error messages then.
@hungrybluedev what do you think?

@phoreverpheebs
Copy link
Contributor Author

@medvednikov maybe a checked version (just as we did with division)? Mainly so the user has a choice and isn't forced to use a method that could panic?

@medvednikov
Copy link
Member

medvednikov commented Jul 21, 2023 via email

@phoreverpheebs phoreverpheebs changed the title math.big: make isqrt return Result instead of runtime panic math.big: add isqrt_checked and standardize error format Jul 21, 2023
Comment on lines +70 to +71
a_operand := integer_from_string('95484736384949495947362') or { panic(err) }
b_operand := integer_from_string('39474638493') or { panic(err) }
Copy link
Member

Choose a reason for hiding this comment

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

inside test functions, you can just use ! instead of or { panic(err) }

@spytheman spytheman merged commit dcbc9e0 into vlang:master Jul 22, 2023
41 checks passed
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.

None yet

3 participants