-
Notifications
You must be signed in to change notification settings - Fork 16
[Guideline] Add do not divide by 0 #132
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for scrc-coding-guidelines ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hi @vapdrs! I have already sent you a message in Zulip, but here seems like a better place to do so. I come to add a couple of things to this guideline :)
There are other such operations for division, such as And for other arithmetic operations, there are quite a few functions one can use to avoid Undefined Behavior: https://doc.rust-lang.org/std/?search=checked
This combines rather well with Option, as in Option<NonZero>, since the compiler can do some memory layout optimization due to the fact that the value being enclosed by NonZero has one bit-pattern that is known to not be possible (the 000...000 pattern) I will review the PR shortly :3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this up @vapdrs! Could you check the comments I left?
As stated there is no compliant way to do this, so no example should be present.
While the guideline does not strictly apply to this example, it is a good suggestion for what to do instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pulling this together @vapdrs! I left a few suggestions based on how conf.py
works together with Sphinx Needs as well as a way to use our coding guidelines extension. Could you take a look?
|
||
.. guideline:: Do not divide by 0 | ||
:id: gui_kMbiWbn8Z6g5 | ||
:category: Mandatory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:category: Mandatory | |
:category: mandatory |
Should be lowercase, see here in conf.py
.
:status: draft | ||
:release: latest | ||
:fls: fls_Q9dhNiICGIfr | ||
:decidability: Undecidable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:decidability: Undecidable | |
:decidability: undecidable |
Should be lowercase, see here in conf.py
.
:release: latest | ||
:fls: fls_Q9dhNiICGIfr | ||
:decidability: Undecidable | ||
:scope: System |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:scope: System | |
:scope: system |
Should be lowercase, see here in conf.py
.
:status: draft | ||
|
||
There is no compliant way to perform integer division by zero. A checked division will prevent any | ||
division by zero from happening. The programmer can then handle the returned Option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out how we can use one of the nice bits of the coding guidelines extension to link to the Rust standard library.
division by zero from happening. The programmer can then handle the returned Option. | |
division by zero from happening. The programmer can then handle the returned :std:``std::option::Option``. |
(This was shamelessly stolen from the FLS extension)
@@ -81,3 +81,44 @@ Expressions | |||
} | |||
|
|||
fn with_base(_: &Base) { ... } | |||
|
|||
.. guideline:: Do not divide by 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking a bit of a practical question to folks. Does the combination of:
mandatory
undecidable
put too great of a burden on reviewers / auditors or is this "just the way it is and should be" given the nature of writing safety-critical code?
Tagging @AlexCeleste and @rcseacord for their thoughts as well.
Closes #131