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

Be stricter with side effects in VERIFY #1485

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

Conversation

real-or-random
Copy link
Contributor

Adds a rule to CONTRIBUTING.md and makes the code adhere to it.

@real-or-random real-or-random added assurance refactor/smell meta/development processes, conventions, developer documentation, etc. labels Jan 17, 2024
@real-or-random
Copy link
Contributor Author

cc @theStack because you worked on #1393

size_t i_ver;
for (i_ver = 0; i_ver < len; i_ver++) {
SECP256K1_GE_VERIFY(&r[i_ver]);
}
}
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These loops caught my attention initially. Here we set i inside VERIFY block, but we also use it outside. I think this should be avoided out of an abundance of caution.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Concept ACK

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@theStack theStack 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 f65f1a4

@real-or-random
Copy link
Contributor Author

thanks, rebased.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

re-ACK f3b91f0

src/modules/ellswift/main_impl.h Outdated Show resolved Hide resolved
src/modules/ellswift/main_impl.h Outdated Show resolved Hide resolved
Adds a rule to CONTRIBUTING.md and makes the code adhere to it.
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

re-ACK ee7083f
(only whitespace fixes since my previous ACK)

@bitcoin-core bitcoin-core deleted a comment from 88woods88 Aug 5, 2024
@bitcoin-core bitcoin-core deleted a comment from 88woods88 Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assurance meta/development processes, conventions, developer documentation, etc. refactor/smell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants