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

pe: simplify generate_hash() #411

Merged
merged 1 commit into from
Sep 10, 2021
Merged

pe: simplify generate_hash() #411

merged 1 commit into from
Sep 10, 2021

Conversation

xypron
Copy link
Contributor

@xypron xypron commented Sep 3, 2021

Copying the value of datasize_in to two further variables and then using
all three randomly in the code makes it hard to read.

datasize_in is never changed in generate_hash() so we can do with this
parameter alone. Let's declare it as const to verify that its value
is invariant.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

@xnox
Copy link
Contributor

xnox commented Sep 6, 2021

Given that there is no "datasize_out" and that most of the function code uses "datasize" maybe it's better to rename the function argument to "datasize" and then drop all references to "size" and "datasize_in"? this should make the diff smaller, no?

@xypron
Copy link
Contributor Author

xypron commented Sep 8, 2021

@xnox: Thank you for reviewing. I will update the patch and force push it.

@xypron
Copy link
Contributor Author

xypron commented Sep 8, 2021

@xnox: I have update the patch according to your recommendation. Please, review again.

@xypron
Copy link
Contributor Author

xypron commented Sep 8, 2021

Rebased on current origin/main

Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

While the rest of the code is fine, I don't think making the function parameter const increases readability, or really gains anything. If anything, it's unusual enough that it makes the function harder to read in my opinion.

Code seems fine otherwise.

@xypron xypron force-pushed the generate_hash branch 2 times, most recently from cd6660d to 3e68352 Compare September 10, 2021 15:03
Copying the value of datasize_in to two further variables and then using
all three randomly in the code makes it hard to read.

datasize_in is never changed in generate_hash() so we can do with this
parameter alone. Rename it to datasize.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
@xypron
Copy link
Contributor Author

xypron commented Sep 10, 2021

@frozencemetery: thank you for reviewing. I removed const and force pushed to corrected patch.

Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

Thanks, I'm happy with this.

@vathpela vathpela merged commit 2699836 into rhboot:main Sep 10, 2021
@xypron xypron deleted the generate_hash branch December 3, 2021 17:34
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.

4 participants