Remove gotos in seeder/dns.cpp
AbandonedPublic
Actions

Authored by nakihito on Dec 4 2019, 21:12.

Details

Reviewers

deadalnix
Fabien
jasonbcox

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

gotos make for bad, error prone, and hard to follow/read code.
Replacing these improves the overall code quality and makes it easier to
follow/read.

In this case, there are numerous gotos that share the exact same label error. This name is also shared with variable names further complicating the code. In addition, having to scroll back and forth between the error label and its associated goto makes reading the code more difficult, especially with all the instances of error that occur in this file. Finally, the code following the error labels can be refactored to or are only 1-2 lines of code while removing unnecessary variables.

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

RemoveGotos

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8420
Build 14855: Default Diff Build & Tests
Build 14854: arc lint + arc unit

Event Timeline

nakihito created this revision.Dec 4 2019, 21:12

Owners added a reviewer: Restricted Owners Package.Dec 4 2019, 21:12

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 4 2019, 21:12

Build Bitcoin-ABC / Diffs / Diff Testing started.

jasonbcox requested changes to this revision.Dec 4 2019, 21:22

jasonbcox added inline comments.

src/seeder/dns.cpp
213 ↗	(On Diff #14623)	wow multiple labels of the same name in the same file? This GOTO setup is truly evil.
354 ↗	(On Diff #14623)	If the goal is readability, then this really should be one memset rather than 8 lines.

This revision now requires changes to proceed.Dec 4 2019, 21:22

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8412: Diff 14623.Dec 4 2019, 21:22

Added constant for counts size and changed multiple assignments to one memset.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8420: Diff 14635.Dec 5 2019, 00:45

Fabien requested changes to this revision.Dec 5 2019, 08:03

Fabien added inline comments.

src/seeder/dns.cpp
167	You don't need error at all
203	Dito
347	This is not doing the same thing
376	Dito, you could update you function and `return setErrorAndZeroCounts(1, outbuf);`

This revision now requires changes to proceed.Dec 5 2019, 08:03

Removed unnecessary error variables and streamlined returns.

nakihito planned changes to this revision.Dec 5 2019, 20:27

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8437: Diff 14662.Dec 5 2019, 20:38

Using goto to not litter otherwise good code with error handling is a fairly common practice. Surely, if this makes this code bad in some way, the specifics can be pointed at rather than blanket statement about goto being harmful.

This guy seems to think goto for error handling is fine: https://stackoverflow.com/a/24476/672906
This guy as well: https://www.cprogramming.com/tutorial/goto.html
Or that other guy (you have heard of him): https://lkml.org/lkml/2003/1/12/128

I would recommend that you'd be able to explain why the use of goto in that piece of code is a problem, because it seems to be the established way to do things in C. What is it that you are getting out of this?

As far as I can tell, this is exposing more implementation details to 3rd parties, and changing a bunch of fragile - and untested - logic. Previous iterations of that patch already contained errors. The benefits are unclear to me.

It could be argue that the C way is error prone to begin with, and I tend to agree, but this patch clearly is not changing this.

deadalnix requested changes to this revision.Dec 5 2019, 21:59

nakihito edited the summary of this revision. (Show Details)Dec 9 2019, 23:00

nakihito abandoned this revision.Jun 23 2020, 22:21