Removed gotos, which are hard to trace and maintain. #213

pull sipa wants to merge 1 commits into bitcoin-core:master from sipa:goto3 changing 1 files +5 −10

sipa commented at 12:51 AM on February 13, 2015: contributor

Rebased version of #97.
sipa cross-referenced this on Feb 13, 2015 from issue Goto3 by iangfc
Removed gotos, which are hard to trace and maintain. 11690d3bb9
sipa force-pushed on Feb 13, 2015
gmaxwell commented at 2:31 AM on February 13, 2015: contributor

utACK.
sipa merged this on Feb 13, 2015
sipa closed this on Feb 13, 2015
sipa referenced this in commit 7b2fc1cac6 on Feb 13, 2015
in src/secp256k1.c:None in 11690d3bb9
```
  44 | @@ -46,20 +45,16 @@ int secp256k1_ecdsa_verify(const unsigned char *msg32, const unsigned char *sig,
  45 |      secp256k1_scalar_set_b32(&m, msg32, NULL);
  46 |  
  47 |      if (!secp256k1_eckey_pubkey_parse(&q, pubkey, pubkeylen)) {
  48 | -        ret = -1;
  49 | -        goto end;
  50 | +        return -1;
```
droark commented at 5:12 PM on February 14, 2015:

(Caveat: I know this code has already been committed. I didn't see the pull request until just now.)

If maintenance is a concern, wouldn't a slightly better solution be to return to the original code, ditch the gotos, and switch the if statements to if/else if/else if? That way, the code doesn't become littered with return statements. I was taught growing up that, whenever possible, it's best to write code that has only one return statement in order to make it easier to read and debug.

Thanks.

gmaxwell commented at 12:41 AM on February 15, 2015:

to quote myself before:

03:21 <gmaxwell> Would you consider this a good alternative to Ian's goto elimination patch? https://0bin.zertrin.org/paste/757a01c8686d7258013d707101734d4121dbee5c#Lk85xk9ip5I9LXVPBoOLOGiJH25ZKTVXLTWB1HDVUJg= it also avoids the multiple returns (which are really just goto in disguise, and can be problematic for the same reasons)

sipa commented at 4:21 AM on February 15, 2015:

This paste has expired.

sipa commented at 4:21 AM on February 15, 2015:

Anyway, sure; patches welcome

Contributors

Linked (view graph)