Refactor/solve clippy warnings

[danielorihuela]

Description

Solve clippy warning. Related to #35.

Checklist

• [x] I have followed the Contribution Guidelines and Code of Conduct
• [x] I have commented my code following the OpenMined Styleguide
• [x] I have labeled this PR with the relevant Type labels
• [x] My changes are covered by tests

[danielorihuela]

@arturomf94 I tried to solve all the clippy warnings. I only have left the warning in the image down below.

I do not have enough knowledge to know which parameters of this function could be joined into a structure. An alternative would be to ignore the warning with: "#[allow(clippy::too_many_arguments)]"

[nph4rd]

Hey! Nice! :) This is real good!

I think these changes broke the benchmarks, however. Do you mind fixing that?

I do not have enough knowledge to know which parameters of this function could be joined into a structure. An alternative would be to ignore the warning with: "#[allow(clippy::too_many_arguments)]"

Regarding, this, I think we could add the #[allow(clippy::too_many_arguments)] for now, and add an issue to request the removal of it. Maybe we can discuss that with @tholop in detail within the issue. WDYT?

[danielorihuela]

Hey! Nice! :) This is real good!

I think these changes broke the benchmarks, however. Do you mind fixing that?

I do not have enough knowledge to know which parameters of this function could be joined into a structure. An alternative would be to ignore the warning with: "#[allow(clippy::too_many_arguments)]"

Regarding, this, I think we could add the #[allow(clippy::too_many_arguments)] for now, and add an issue to request the removal of it. Maybe we can discuss that with @tholop in detail within the issue. WDYT?

Yeah, I saw that I broke them. I do not know why, I am taking a look. From my point of view, I think it is okey to ignore the warning and create an issue until we know what to do.

[nph4rd]

@danielorihuela - noticed there are still some warnings here: https://github.com/OpenMined/sycret/pull/36/checks?check_run_id=3656063175

They're related to using something like x = x + c instead of x += c. Did you omit that on purpose?

[nph4rd]

@danielorihuela - noticed there are still some warnings here: https://github.com/OpenMined/sycret/pull/36/checks?check_run_id=3656063175

They're related to using something like x = x + c instead of x += c. Did you omit that on purpose?

I think it would be nice to include it! :)

[tholop]

Thank you, I left some comments. I'm not sure all the changes are necessary, but I might have missed something.

[danielorihuela]

@danielorihuela - noticed there are still some warnings here: https://github.com/OpenMined/sycret/pull/36/checks?check_run_id=3656063175

They're related to using something like x = x + c instead of x += c. Did you omit that on purpose?

They do not appear to me. Which version of cargo do yo have? I have 1.55.0.

[nph4rd]

@danielorihuela - noticed there are still some warnings here: https://github.com/OpenMined/sycret/pull/36/checks?check_run_id=3656063175 They're related to using something like x = x + c instead of x += c. Did you omit that on purpose?

They do not appear to me. Which version of cargo do yo have? I have 1.55.0.

They are in the link. You can see that they appeared when the workflow ran. For instance, one of them is in tests/test_le.rs:20:9

[nph4rd]

@danielorihuela they can be foun here too: https://github.com/OpenMined/sycret/pull/36/checks?check_run_id=3661355010

They're under the Clippy section.

[tholop]

Nit: what is the point of those empty // # Safety comments? I think that the warning makes sense if we actually use the comment to explain why we need an unsafe block (even in a few words such as "access to Numpy-allocated memory")

[nph4rd]

Cool! There's just one last warning:

warning: unnecessary parentheses around assigned value
  --> tests/test_le.rs:22:18
   |
22 |         alpha -= (-offset as u32);
   |                  ^^^^^^^^^^^^^^^^ help: remove these parentheses
   |
   = note: `#[warn(unused_parens)]` on by default

[danielorihuela]

Nit: what is the point of those empty // # Safety comments? I think that the warning makes sense if we actually use the comment to explain why we need an unsafe block (even in a few words such as "access to Numpy-allocated memory")

I tried to explain the safety conditions. But a double check from both of you could be nice.

Cool! There's just one last warning:

warning: unnecessary parentheses around assigned value
  --> tests/test_le.rs:22:18
   |
22 |         alpha -= (-offset as u32);
   |                  ^^^^^^^^^^^^^^^^ help: remove these parentheses
   |
   = note: `#[warn(unused_parens)]` on by default

It should be solved now.

[nph4rd]

It looks good. There's just one description that I think could be more explicit (see above).

[nph4rd]

@danielorihuela - wondering: are you on the org's Slack?

[danielorihuela]

@danielorihuela - wondering: are you on the org's Slack?

Yes!!! To demonstrate it, I can tell you that you recommended a book in the general channel.