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

Use macro for wrapping += and -= #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Mar 15, 2024

Use macro for wrapping += and -=

The current auto-generated += and -= implementation is hard to read, and should be replaced with += where possible. That said, we cannot auto-replace it just yet because Rust behaves differently in debug mode, therefore we should use second best approach - a macro that clearly shows intention without all the boilerplate code.

The only manual code are two macros in the src/enc/mod.rs

Use this replacement file as described in other recent PRs to replace boilerplate code.

replacement file content
@@
expression cond, expr;
@@
if cond
{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, 1);
}

@@
expression expr;
@@
-{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, 1);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1u32;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs);
+::wrapping_add!(expr, 1);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1i32;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, 1);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as usize);
+::wrapping_add!(expr, 1);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, inc as u32);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs);
+::wrapping_add!(expr, inc);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, inc as u32);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as usize);
+::wrapping_add!(expr, inc as usize);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_sub(_rhs as usize);
+::wrapping_sub!(expr, 1);
-}

@nyurik nyurik force-pushed the add_assign_wrapping branch from e594745 to a8ac00d Compare March 16, 2024 00:51
@nyurik
Copy link
Contributor Author

nyurik commented Mar 16, 2024

i just updated the repl file content above - it was in the git log message, but not here

@danielrh
Copy link
Collaborator

I don't think a macro is the right approach here. You could have a function that takes in a mut ref, but I'm not sure it's more readable than the wrapping add inline expr.

@nyurik
Copy link
Contributor Author

nyurik commented Mar 17, 2024

I can change it to a function, but having one line is much easier to read than 5, and does not hide intentional logic

@nyurik
Copy link
Contributor Author

nyurik commented Mar 17, 2024

@danielrh wrapping_add is not a trait method, so we would have to have several separate functions like wrapping_add_usize, wrapping_add_i32, etc. Moreovere, I suspect that in some of these, we can replace them with += because we can deduce that the number will never wrap, and was simply auto-generated by cross-compiler. What do you think?

@nyurik nyurik force-pushed the add_assign_wrapping branch from a8ac00d to a9482b5 Compare March 17, 2024 20:21
@nyurik nyurik force-pushed the add_assign_wrapping branch 2 times, most recently from 96104a0 to ece67bc Compare March 17, 2024 20:28
@nyurik
Copy link
Contributor Author

nyurik commented Mar 17, 2024

I removed some changes from this PR that were not related to the wrapping - moved to #160

@nyurik nyurik force-pushed the add_assign_wrapping branch 3 times, most recently from aa523c9 to faeafea Compare March 18, 2024 15:34
@nyurik nyurik force-pushed the add_assign_wrapping branch 4 times, most recently from a473a18 to 821b066 Compare April 7, 2024 15:38
@nyurik nyurik force-pushed the add_assign_wrapping branch from 821b066 to 90e2a52 Compare April 18, 2024 07:06
@nyurik nyurik force-pushed the add_assign_wrapping branch from 90e2a52 to 769b39e Compare April 25, 2024 15:41
@nyurik nyurik force-pushed the add_assign_wrapping branch 5 times, most recently from a45907a to c318d7c Compare May 11, 2024 06:36
@nyurik nyurik force-pushed the add_assign_wrapping branch 2 times, most recently from b515add to 9a8c7e6 Compare May 21, 2024 13:13
@nyurik nyurik force-pushed the add_assign_wrapping branch from 9a8c7e6 to f9662e8 Compare May 27, 2024 11:30
The current auto-generated += and -= implementation is hard to read, and should be replaced with += where possible. That said, we cannot auto-replace it just yet because Rust behaves differently in debug mode, therefore we should use second best approach - a macro that clearly shows intention without all the boilerplate code.

The only manual code are two macros in the src/enc/mod.rs

Use this replacement file as described in other recent PRs to replace boilerplate code.

<details>
<summary>replacement file content</summary>

```diff
@@
expression cond, expr;
@@
if cond
{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, 1);
}

@@
expression expr;
@@
-{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, 1);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1u32;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs);
+::wrapping_add!(expr, 1);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1i32;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, 1);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as usize);
+::wrapping_add!(expr, 1);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, inc as u32);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs);
+::wrapping_add!(expr, inc);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as u32);
+::wrapping_add!(expr, inc as u32);
-}

@@
expression inc, expr;
@@
-{
-   let _rhs = inc;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_add(_rhs as usize);
+::wrapping_add!(expr, inc as usize);
-}

@@
expression expr;
@@
-{
-   let _rhs = 1;
-   let _lhs = &mut expr;
-   *_lhs = (*_lhs).wrapping_sub(_rhs as usize);
+::wrapping_sub!(expr, 1);
-}
```

</details>
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.

2 participants