-
Notifications
You must be signed in to change notification settings - Fork 16
[Guideline] Add checked arithmetic guidelines #136
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. |
The |
These correspond to the unique ID's for each section.
.. non_compliant_example:: | ||
:id: non_compl_ex_0XeioBrgfh5z | ||
:status: draft | ||
|
||
When the division is performed, the right operand is evaluated to zero and the program panics. | ||
|
||
.. code-block:: rust | ||
let x = 0; | ||
let x = 5 / x; | ||
.. compliant_example:: | ||
:id: compl_ex_k1CD6xoZxhXb | ||
:status: draft | ||
|
||
If an overflow occurs, it is explicit that the addition should wrap. | ||
|
||
.. code-block:: rust | ||
let y = 135u8 | ||
let x = 200u8.wrapping_add(y); | ||
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.
Is the document allowed to have more than one non-compliant and more than one compliant example?
Since this guideline covers both the divide-by-zero and the arithmetic-overflow cases, I think it's only natural that it would have an example for both :)
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.
you should have a compliant solution corresponding to each noncompliant example. For example, how do I deal with divide by zero on a compliant solution. Also show the incorrect version of the wrapping example.
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.
I'm unsure if multiple examples are allowed, but these are small and related so I just put both in one code block in 752ba7a
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.
We are still at the point where these things can be discussed. If we'd like to have a number of compliant and non-compliant examples, this is possible likely in Sphinx Needs. Opened #141 to track.
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.
Question to @vapdrs, @rcseacord, @felix91gr -- is this something which we think should be addressed as a blocking concern for this PR or mark this as resolved since we have #141 and move on for now?
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.
@PLeVasseur I would much prefer it if we had multiple examples, one for each specific case. Whenever I've read guidelines, having "atomic" (ie indivisible / minimal) examples has been essential for me to understand them quickly and without needing to ask for further help. I think we can benefit a lot if each example addresses a single concern.
So yeah, I think it's best if we block until we have this capability. Sorry, @vapdrs, I'm asking you to wait until we've sorted this
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.
No worries here. I don't mind waiting.
Resolves #20 |
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.
Hey @vapdrs -- appreciate this addition! I've left a few suggestions and comments
:scope: module | ||
:tags: numerics | ||
|
||
This guideline applies when an `ArithmeticExpression` is used with operands of integer type. |
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.
Should ArithmeticExpression
be linked to the relevant section of the FLS? Feel this might be nice. 🙂
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.
Good idea, resolved in 36f60e2
:id: rat_7tF18FIwSYws | ||
:status: draft | ||
|
||
The semantics for these expressions can result in panics, or silent wraparound upon overflow or division |
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.
nit
Suggest either "wrap-around" or "wrap around". Them concatenated together somehow looks a bit off to me 😅
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.
Agreed, resolved in 9dc042f
Co-authored-by: Pete LeVasseur <[email protected]>
Additionally the code block formatting was updating, and a link was added. Co-authored-by: Pete LeVasseur <[email protected]>
:id: rat_7tF18FIwSYws | ||
:status: draft | ||
|
||
The semantics for these expressions can result in panics, or silent wrap around upon overflow or division |
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.
This was probably just copied from above, but it is wrong. The unchecked functions lead to UB when overflow happens, so they are more dangerous than the regular arithmetic expressions.
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.
Thank you for pointing this out, resolved in 2fb316f
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.
You forgot the next sentence. There it also says panic.
As pointed out, they result in UB not necessarily panics
Add two new guidlelines disallowing most ArithmeticExpressions and unchecked arithmetic. The reason there are two rules is such that if a project really needs the performance afforded by
unchecked_mul
, they can disregard the advisory rule and use the required one.