-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Code refractored in order to follow best coding practices #6511
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
Conversation
Travis CI: No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself. |
Travis fails with Looks like a libsecp256k issue (@sipa)? |
@@ -64,11 +64,12 @@ class CValidationState { | |||
return mode == MODE_ERROR; | |||
} | |||
bool IsInvalid(int &nDoSOut) const { | |||
bool isInvalid = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing code which spans over two lines to spread over six lines can only decrease readability.
This PR does the exact opposite of what the "block style example" says in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding |
Too risky for what is gained, IMO. We don't usually merge these kinds of random code refactors because - even when they do improve readability - in the past some very nasty bugs have been introduced by them, accidentally. That these changes are in consensus/validation code compounds the above-mentioned risks. If your goal is to make the code more understandable, adding of comments is preferable. |
Hi! |
@pouta The tests hardly ever cover all possible cases. I agree that there is a class of changes that is too simple to fail, but just as well there are some seeming innocent changes that break bug-for-bug compatibility which is very important in the consensus code. @jonasschnelli opened an issue for that, #6515. Looks like secp256k1 context is the victim, not the cause of the problem. |
Just a small code refractoring. But we should keep only one return in each function in order to improve readability and usability.