Skip to content

Trivial ChaCha cleanups #2764

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

I was looking at this code and it bothered me.

While its all constant arithmetic to calculate the shift, which
LLVM likely optimizes out for us, there's no reason to do it four
times, which just makes the code harder to read.
These are obviously super hot, and while LLVM shouldn't be
braindead here you never know, so we might as well `#[inline]`.
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f07f4b9) 88.57% compared to head (c79cf82) 88.57%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2764   +/-   ##
=======================================
  Coverage   88.57%   88.57%           
=======================================
  Files         115      115           
  Lines       89479    89479           
  Branches    89479    89479           
=======================================
+ Hits        79258    79260    +2     
+ Misses       7858     7852    -6     
- Partials     2363     2367    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jkczyz
Copy link
Contributor

jkczyz commented Dec 1, 2023

FYI, we copied this code in vss-rust-client: lightningdevkit/vss-rust-client#14 (comment)

Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Code Review ACK

  • Out of all occurrences of >> or << operation, there are no instances where any of the rotation matrix's u32x4 elements are different from each other.
  • In this case, it makes sense to simplify the code, making it easier to understand and reason with.

@shaavan
Copy link
Member

shaavan commented Dec 2, 2023

FYI, copied this code in vss-rust-client

This PR is a definite improvement over the current code, and I feel like it's best to maintain consistency between the various codebases under LDK org.

@TheBlueMatt
Copy link
Collaborator Author

FYI, we copied this code in vss-rust-client

Yea, there's nothing wrong with the code, 90% chance LLVM optimizes it down into identical code anyway, but I just felt like updating it cause it was ugly :)

@valentinewallace
Copy link
Contributor

Pretty trivial and this code should be hit in most tests as well, so I'll land.

@valentinewallace valentinewallace merged commit 37150b4 into lightningdevkit:main Dec 5, 2023
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.

5 participants