Page MenuHomePhabricator
Differential D5471

Add enum for parse_name() return value
ClosedPublic
Actions

Authored by nakihito on Mar 12 2020, 21:13.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING939971dd5e02: Add enum for parse_name() return value
rABC939971dd5e02: Add enum for parse_name() return value
Summary

Currently, parse_name() returns an int that doesn't describe what has
actually happened (at least without reading the comment that was in
dns.h). This changes the return value to something more descriptive.

Depends on D5470

Test Plan
ninja check-bitcoin-seeder

Run sanitizer builds

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ParseNameEnum
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9803
Build 17479: Default Diff Build & Tests
Build 17478: arc lint + arc unit

Event Timeline

nakihito created this revision.Mar 12 2020, 21:13
Owners added a reviewer: Restricted Owners Package.Mar 12 2020, 21:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 12 2020, 21:13
teamcity added a comment.Mar 12 2020, 21:13

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Mar 12 2020, 21:23

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9801: Diff 16910.Mar 12 2020, 21:23
jasonbcox requested changes to this revision.Mar 13 2020, 04:06
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/seeder/dns.h
32 ↗(On Diff #16910)

Make this an enum class so that it's strongly typed.

Also, does the underlying type have any significance? My guess is no considering the values changed.

src/seeder/test/dns_tests.cpp
83 ↗(On Diff #16910)

Isn't this going to run into the same issue as D5470 ?

This revision now requires changes to proceed.Mar 13 2020, 04:06
teamcity added a comment.Mar 13 2020, 04:14

Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) started.

nakihito planned changes to this revision.Mar 13 2020, 04:18
nakihito added inline comments.
src/seeder/dns.h
32 ↗(On Diff #16910)

No, the underlying type doesn't have any behavioral use.

src/seeder/test/dns_tests.cpp
83 ↗(On Diff #16910)

You're probably right.

teamcity added a comment.Mar 13 2020, 04:29
Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) failed.

Snippet of first build failure:

[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:189:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(int __n);
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:166:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(long __n)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:170:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(unsigned long __n)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:178:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(short __n);
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:181:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(unsigned short __n)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:192:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(unsigned int __n)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:201:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(long long __n)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:205:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(unsigned long long __n)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:220:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(double __f)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:224:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(float __f)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:232:7: note: candidate function
[04:28:52] :	 [Step 1/1]       operator<<(long double __f)
[04:28:52] :	 [Step 1/1]       ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:508:5: note: candidate function [with _Traits = std::char_traits<char>]
[04:28:52] :	 [Step 1/1]     operator<<(basic_ostream<char, _Traits>& __out, char __c)
[04:28:52] :	 [Step 1/1]     ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:502:5: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>]
[04:28:52] :	 [Step 1/1]     operator<<(basic_ostream<_CharT, _Traits>& __out, char __c)
[04:28:52] :	 [Step 1/1]     ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:514:5: note: candidate function [with _Traits = std::char_traits<char>]
[04:28:52] :	 [Step 1/1]     operator<<(basic_ostream<char, _Traits>& __out, signed char __c)
[04:28:52] :	 [Step 1/1]     ^
[04:28:52] :	 [Step 1/1] /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ostream:519:5: note: candidate function [with _Traits = std::char_traits<char>]
[04:28:52] :	 [Step 1/1]     operator<<(basic_ostream<char, _Traits>& __out, unsigned char __c)
[04:28:52] :	 [Step 1/1]     ^
[04:28:52] :	 [Step 1/1] 2 errors generated.
[04:28:53] :	 [Step 1/1] [6/27] Test Bitcoin utilities...
[04:28:53] :	 [Step 1/1] [7/27] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[04:28:54] :	 [Step 1/1] [8/27] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[04:28:54] :	 [Step 1/1] [9/27] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[04:28:54] :	 [Step 1/1] [10/27] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[04:28:54] :	 [Step 1/1] ninja: build stopped: subcommand failed.
[04:28:54] :	 [Step 1/1] *** Output of /tmp/sanitizer_logs/*.log.* ***
[04:28:54]W:	 [Step 1/1] ++ print_sanitizers_log
[04:28:54]W:	 [Step 1/1] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[04:28:54]W:	 [Step 1/1] ++ echo '*** Output of /tmp/sanitizer_logs/*.log.* ***'
[04:28:54]W:	 [Step 1/1] ++ cat '/tmp/sanitizer_logs/*.log.*'
[04:28:54]W:	 [Step 1/1] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[04:28:54]W:	 [Step 1/1] Process exited with code 1
[04:28:55]E:	 [Step 1/1] Process exited with code 1 (Step: Command Line)
nakihito updated this revision to Diff 16913.Mar 13 2020, 04:39

Changed enum to enum class and added << operator for sanitizer builds.

nakihito planned changes to this revision.Mar 13 2020, 04:39
teamcity added a comment.Mar 13 2020, 04:39

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Mar 13 2020, 04:40

Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) started.

teamcity added a comment.Mar 13 2020, 05:02

Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) passed.

teamcity added a comment.Mar 13 2020, 05:03

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9803: Diff 16913.Mar 13 2020, 05:03
nakihito requested review of this revision.Mar 13 2020, 05:11
deadalnix requested changes to this revision.Mar 13 2020, 07:33
deadalnix added a subscriber: deadalnix.

There are some improvements you can d here, but overall it's good.

src/seeder/dns.cpp
66

I saw that pattern in another patch, which kind of confirm that the change I requested there were correct. See https://reviews.bitcoinabc.org/D5470#inline-33740

src/seeder/dns.h
10

Use the most restrictive header, not iostream.

src/seeder/test/dns_tests.cpp
61

It is beyond the scope of that patch, but checking for the proper value would be better than this.

This revision now requires changes to proceed.Mar 13 2020, 07:33
jasonbcox mentioned this in D5470: Add << operator overload for PeerMessagingState.Mar 13 2020, 17:41
nakihito updated this revision to Diff 16930.Mar 13 2020, 20:44

Rebased off D5470.

teamcity added a comment.Mar 13 2020, 20:44

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

nakihito edited the summary of this revision. (Show Details)Mar 13 2020, 20:45
nakihito edited the test plan for this revision. (Show Details)
nakihito added a parent revision: D5470: Add << operator overload for PeerMessagingState.
teamcity added a comment.Mar 13 2020, 20:45

Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) started.

teamcity added a comment.Mar 13 2020, 20:54

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9814: Diff 16930.Mar 13 2020, 20:54
teamcity added a comment.Mar 13 2020, 21:07

Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) passed.

deadalnix accepted this revision.Mar 14 2020, 10:30
jasonbcox accepted this revision.Mar 14 2020, 22:03
jasonbcox added inline comments.
src/seeder/dns.h
32 ↗(On Diff #16930)

Nit: No need to set the underlying type since it isn't used.

This revision is now accepted and ready to land.Mar 14 2020, 22:03
nakihito updated this revision to Diff 16948.Mar 16 2020, 17:27

Rebased.

teamcity added a comment.Mar 16 2020, 17:27

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

teamcity added a comment.Mar 16 2020, 17:27

Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) started.

teamcity added a comment.Mar 16 2020, 17:36

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B9825: Diff 16948.Mar 16 2020, 17:36
teamcity added a comment.Mar 16 2020, 17:55

Build Bitcoin-ABC / Diffs / Diff Testing (build-asan) passed.

Closed by commit rABC939971dd5e02: Add enum for parse_name() return value (authored by nakihito). · Explain WhyMar 16 2020, 18:10
This revision was automatically updated to reflect the committed changes.
nakihito added a commit: rABC939971dd5e02: Add enum for parse_name() return value.
nakihito added a commit: rSTAGING939971dd5e02: Add enum for parse_name() return value.Apr 7 2020, 23:20

Revision Contents
Changeset List

PathSize
src/
seeder/
20 lines
37 lines
test/
34 lines

Diff 16913

View Options

src/seeder/dns.h

Loading...
View Options

src/seeder/dns.cpp

Loading...
View Options

src/seeder/test/dns_tests.cpp

Loading...