refactor: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845) #20033

pull vasild wants to merge 1 commits into bitcoin:master from vasild:nits_followup_to_19845 changing 2 files +7 −4

vasild commented at 10:15 AM on September 29, 2020: contributor

Address suggestions: #19845 (review) #19845 (review) #19845 (review)
vasild commented at 10:16 AM on September 29, 2020: contributor

cc @hebasto
vasild cross-referenced this on Sep 29, 2020 from issue net: CNetAddr: add support to (un)serialize as ADDRv2 by vasild
hebasto commented at 10:21 AM on September 29, 2020: member

Concept ACK, obviously :) Thanks @vasild !

Mind also integrating #19845 (review) as that change was unneeded?
vasild commented at 10:37 AM on September 29, 2020: contributor
Well, I left it as is because
```
template <typename E>
bool operator()(const E& e) const {
```
is more generic than
```
bool operator()(const std::runtime_error& e) const {
```
both work now but the former would also work if the exception does not inherit std::runtime_error.
fanquake added the label P2P on Sep 29, 2020
hebasto commented at 10:58 AM on September 29, 2020: member
I don't think the template function is required to make exception handling more generic for the following reasons:
- all custom types in the project inherit from std::runtime_error
- if this seems insufficient, the base std::exception type could be used
ajtowns commented at 8:57 PM on October 6, 2020: contributor

Please describe the actual changes in the pr title/description?
fanquake added the label Waiting for author on Oct 31, 2020
fanquake commented at 12:31 PM on October 31, 2020: member

@vasild could you follow up here with @hebasto and @ajtowns comments, as this is a fairly straight-forward change.
vasild renamed this:
~~style: minor improvements as a followup to #19845~~
style: minor whitespace fixups and s/const/constexpr/ (followup to #19845)
on Oct 31, 2020
vasild force-pushed on Oct 31, 2020
vasild renamed this:
~~style: minor whitespace fixups and s/const/constexpr/ (followup to #19845)~~
style: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845)
on Oct 31, 2020
vasild commented at 2:09 PM on October 31, 2020: contributor

Done!
jonatack commented at 2:30 PM on October 31, 2020: contributor

ACK b76317461e4664ba2d2d06c528a67d8718aa7c63

For the PR title, maybe s/style/net, test/ per CONTRIBUTING.md. Feel free to ignore.
hebasto approved
hebasto commented at 2:41 PM on October 31, 2020: member

ACK b76317461e4664ba2d2d06c528a67d8718aa7c63, I have reviewed the code and it looks OK, I agree it can be merged.
vasild renamed this:
~~style: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845)~~
net, test: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845)
on Oct 31, 2020
vasild commented at 3:58 PM on October 31, 2020: contributor

./test/util/setup_common.h:166:10: note: no known conversion for argument 1 from ‘const std::ios_base::failure’ to ‘const std::runtime_error&’ :bomb:

https://travis-ci.org/github/bitcoin/bitcoin/jobs/740417628#L2831

style: minor improvements as a followup to #19845

Address suggestions:
https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495486760
https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495488051
https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495730125

89836a82ee

vasild force-pushed on Oct 31, 2020
jonatack commented at 4:18 PM on October 31, 2020: contributor

Interesting. I compiled and ran the unit tests without warnings.

jonatack commented at 5:04 PM on October 31, 2020: contributor

re-ACK 89836a8 change since previous review is replacing std::runtime_error with std::exception, built/ran unit tests with gcc debian 10.2.0-15, then broke a few v3 net_tests involving BOOST_CHECK_EXCEPTION, rebuilt, ran src/test/test_bitcoin -t net_tests -l all and checked the error reporting.

Change per git diff b763174 89836a8 since previous review:

diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 33c2ea0028..1812ce1666 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -163,7 +163,7 @@ class HasReason
 {
 public:
     explicit HasReason(const std::string& reason) : m_reason(reason) {}
-    bool operator()(const std::runtime_error& e) const
+    bool operator()(const std::exception& e) const
     {
         return std::string(e.what()).find(m_reason) != std::string::npos;
     };

bitcoin deleted a comment on Oct 31, 2020
hebasto commented at 10:59 PM on October 31, 2020: member

re-ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d

./test/util/setup_common.h:166:10: note: no known conversion for argument 1 from ‘const std::ios_base::failure’ to ‘const std::runtime_error&’ bomb

https://travis-ci.org/github/bitcoin/bitcoin/jobs/740417628#L2831

CentOS build uses pretty old gcc. Nice that CI caught that issue.
MarcoFalke removed the label Waiting for author on Nov 1, 2020
bitcoin deleted a comment on Nov 11, 2020
theStack approved
theStack commented at 9:55 PM on November 15, 2020: contributor

ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d
MarcoFalke added the label Refactoring on Nov 16, 2020
MarcoFalke renamed this:
~~net, test: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845)~~
refactor: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845)
on Nov 16, 2020
MarcoFalke merged this on Nov 16, 2020
MarcoFalke closed this on Nov 16, 2020
sidhujag referenced this in commit 806ac5fe66 on Nov 16, 2020
Fuzzbawls cross-referenced this on Aug 12, 2021 from issue [Refactor] Clang-tidy and fix compiler error in HasReason class by Fuzzbawls
furszy referenced this in commit 51055cbd51 on Aug 12, 2021
Fabcien referenced this in commit df99eb6da0 on Dec 23, 2021
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19845 net: CNetAddr: add support to (un)serialize as ADDRv2