Page MenuHomePhabricator

Add enum for parse_name() return value
ClosedPublic

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 12 2020, 21:13
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
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.

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)

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

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 ↗(On Diff #16913)

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 ↗(On Diff #16913)

Use the most restrictive header, not iostream.

src/seeder/test/dns_tests.cpp
61 ↗(On Diff #16913)

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
nakihito edited the test plan for this revision. (Show Details)
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
This revision was automatically updated to reflect the committed changes.