Page MenuHomePhabricator

Add enum for parse_name() return value
ClosedPublic

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

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Thu, Mar 12, 21:13
Owners added a reviewer: Restricted Owners Package.Thu, Mar 12, 21:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Mar 12, 21:13
jasonbcox requested changes to this revision.Fri, Mar 13, 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.Fri, Mar 13, 04:06
nakihito planned changes to this revision.Fri, Mar 13, 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.

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.Fri, Mar 13, 04:39

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

nakihito planned changes to this revision.Fri, Mar 13, 04:39
nakihito requested review of this revision.Fri, Mar 13, 05:11
deadalnix requested changes to this revision.Fri, Mar 13, 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.Fri, Mar 13, 07:33
nakihito updated this revision to Diff 16930.Fri, Mar 13, 20:44

Rebased off D5470.

nakihito edited the summary of this revision. (Show Details)Fri, Mar 13, 20:45
nakihito edited the test plan for this revision. (Show Details)
deadalnix accepted this revision.Sat, Mar 14, 10:30
jasonbcox accepted this revision.Sat, Mar 14, 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.Sat, Mar 14, 22:03
nakihito updated this revision to Diff 16948.Mon, Mar 16, 17:27

Rebased.

This revision was automatically updated to reflect the committed changes.