Code refractored in order to follow best coding practices #6511

pull defijesus wants to merge 4 commits into bitcoin:master from defijesus:master changing 2 files +13 −11

defijesus commented at 4:11 AM on August 4, 2015: none

Just a small code refractoring. But we should keep only one return in each function in order to improve readability and usability.
Code refractored in order to follow best coding practices 57c5c66110
Forgot to return variable that was added. 09df3b3472
More code refracting. Removed multiple redundant returns. 281ea5873b
Forgot to remove one line. 10d75b0ae2
defijesus commented at 4:56 AM on August 4, 2015: none

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.
jonasschnelli commented at 6:22 AM on August 4, 2015: contributor

Travis fails with test_bitcoin: key.cpp:198: void ECC_Start(): Assertion secp256k1_context == __null' failed. unknown location(0): fatal error in "script_PushData": signal: SIGABRT (application abort requested)` Problem location: https://github.com/bitcoin/bitcoin/blob/master/src/key.cpp#L198

Looks like a libsecp256k issue (@sipa)? Same travis problem in #6509.

in src/consensus/validation.h:None in 10d75b0ae2

  63 | @@ -64,11 +64,12 @@ class CValidationState {
  64 |          return mode == MODE_ERROR;
  65 |      }
  66 |      bool IsInvalid(int &nDoSOut) const {
  67 | +        bool isInvalid = false;

MarcoFalke commented at 7:10 AM on August 4, 2015:

Replacing code which spans over two lines to spread over six lines can only decrease readability.

MarcoFalke commented at 7:12 AM on August 4, 2015: member

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
laanwj commented at 11:08 AM on August 4, 2015: member

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.
laanwj closed this on Aug 4, 2015
defijesus commented at 1:20 PM on August 4, 2015: none

Hi! @laanwj I agree this is a small change in a very critical code. But if test pass and build pass. How does adding a new variable to store the return value should produce any bugs? (This kind of change is too simple to fail). Thank you so much for reviewing
laanwj commented at 2:15 PM on August 4, 2015: member

@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.
MarcoFalke cross-referenced this on Sep 10, 2015 from issue [Qt] minor optimisations in peertablemodel by Diapolo
bitcoin locked this on Sep 8, 2021

Contributors

Linked (view graph)

#6317 [Qt] minor optimisations in peertablemodel #6509 Fix race condition on test node shutdown #6515 Intermittent travis failures due to potential_deadlock_detected