Page MenuHomePhabricator

Remove gotos in seeder/dns.cpp
AbandonedPublic

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

Owners added a reviewer: Restricted Owners Package.Dec 4 2019, 21:12
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

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

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.

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