Skip to content

Panic on division by zero - #57

Open
aritkulova wants to merge 5 commits into
devfrom
feat/panic-on-zero-div
Open

aritkulova wants to merge 5 commits into
devfrom
feat/panic-on-zero-div

Conversation

@aritkulova

Copy link
Copy Markdown
Collaborator
  • This PR suggests a bug fix and I've added the necessary tests.
  • This PR introduces a new feature and I've discussed the update in an Issue or with the team.
  • This PR is just a minor change like a typo fix.

Refactors division (including mul_div) functions in the std library to panic on division by zero by default.

@aritkulova aritkulova self-assigned this Sep 22, 2026
@aritkulova

Copy link
Copy Markdown
Collaborator Author

For now, 2 test are failing due to BlockstreamResearch/smplx#151

@aritkulova

Copy link
Copy Markdown
Collaborator Author

Running tests with the -v flag helped (all tests were successful), but it feels like a temporary solution.

@aritkulova aritkulova mentioned this pull request Sep 23, 2026
3 tasks
@apoelstra

Copy link
Copy Markdown

In 99f2449:

This is a 14000-line diff which does multiple things, none of which seem related to this PR.

@schoen

schoen commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Hi, I was going to file a bug report about inconsistent division by zero behavior before I learned that you were working on this. It looks like you have fixed the inconsistent behavior!

Two questions from my end: (1) Did you mean to remove the 128-bit and 256-bit versions of the functions? (That's fine with me if it was intentional.) (2) Could you double check the docs updates? It looks like you've accidentally deleted the documentation for checked_div_256 instead of for safe_div_256.

@apoelstra

Copy link
Copy Markdown

In a1ccb94:

"Underflow" is not the correct word for when subtraction overflows. "Underflow" is about floating-point values rounding to zero. (I know this was present in the original code, but highlighting it now.)

The code changes in this commit look fine to me, other than the deleted functions, formatting changes, deleted tests, etc.

@apoelstra

Copy link
Copy Markdown

I think this PR would be much clearer if it first changed tests to avoid division by 0, then refactored the "big" division methods to never pass 0 to the basic ones, and then updated the basic ones, and then added tests for the corrected division by 0 behavior.

@aritkulova

Copy link
Copy Markdown
Collaborator Author

@schoen, hi

  1. Yes, I meant to remove safe_div_128 and safe_div_256 because we agreed that the general approach in std to division by zero is to panic. The "default" division functions already panic, so safe_div_128 and safe_div_256 just duplicate div_128 and div_256.

Although, now that I think about it, it may be better to rename all safe_div functions to just div functions (safe_div_8->div_8`, for example), both for consistency and to encourage users to use them by default instead of the jets.

  1. Oops, that's my bad. Thank you!

added -v workaround to ci;

fixed typos
@aritkulova
aritkulova force-pushed the feat/panic-on-zero-div branch from aa40069 to 2db0182 Compare September 24, 2026 13:54
Comment thread simf/lib/u128/math.simf Outdated
@Hrom131

Hrom131 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

@aritkulova

I think it's a good idea to have a consistent API for all uint types. It's very inconvenient for developers to use the safe_div_x functions for u8-64 types and the div_x functions for u128-256 types. This raises additional questions. Removing all the safe_ prefixes and making all functions safe by default sounds like a good idea.

@aritkulova

Copy link
Copy Markdown
Collaborator Author

@Hrom131 Cool! I think I'll make a separate PR to remove the safe_ prefixes then, so as not to overload the current PR

@Hrom131 Hrom131 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

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.

4 participants