Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 16, 2020
Merged

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

merged 1 commit into from
Nov 16, 2020

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Sep 29, 2020 •
edited
Loading

@vasild
Copy link
Contributor Author

vasild commented Sep 29, 2020

cc @hebasto

@vasild vasild mentioned this pull request Sep 29, 2020
Merged
@hebasto
Copy link
Member

hebasto commented Sep 29, 2020

Concept ACK, obviously :) Thanks @vasild !

Mind also integrating #19845 (comment) as that change was unneeded?

@vasild
Copy link
Contributor Author

vasild commented Sep 29, 2020

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 fanquake added the P2P label Sep 29, 2020
@hebasto
Copy link
Member

hebasto commented Sep 29, 2020

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
Copy link
Contributor

ajtowns commented Oct 6, 2020

Please describe the actual changes in the pr title/description?

@fanquake
Copy link
Member

@vasild could you follow up here with @hebasto and @ajtowns comments, as this is a fairly straight-forward change.

@vasild vasild changed the title style: minor improvements as a followup to #19845 style: minor whitespace fixups and s/const/constexpr/ (followup to #19845) Oct 31, 2020
@vasild vasild force-pushed the nits_followup_to_19845 branch from 7db9bf3 to b763174 Compare October 31, 2020 14:08
@vasild vasild changed the title style: minor whitespace fixups and s/const/constexpr/ (followup to #19845) style: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845) Oct 31, 2020
@vasild
Copy link
Contributor Author

vasild commented Oct 31, 2020

Done!

jonatack
jonatack reviewed Oct 31, 2020
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b763174

For the PR title, maybe s/style/net, test/ per CONTRIBUTING.md. Feel free to ignore.

hebasto
hebasto approved these changes Oct 31, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b763174, I have reviewed the code and it looks OK, I agree it can be merged.

@vasild vasild changed the title 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) Oct 31, 2020
@vasild
Copy link
Contributor Author

vasild commented Oct 31, 2020 •
edited
Loading

./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&’ 💣

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

@vasild vasild force-pushed the nits_followup_to_19845 branch from b763174 to 89836a8 Compare October 31, 2020 16:03
@jonatack
Copy link
Member

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

@jonatack
Copy link
Member

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 bitcoin deleted a comment from rmtnong Oct 31, 2020
@hebasto
Copy link
Member

hebasto commented Oct 31, 2020

re-ACK 89836a8

./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.

@bitcoin bitcoin deleted a comment from rmtnong Nov 11, 2020
theStack
theStack approved these changes Nov 15, 2020
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 89836a8

@maflcko maflcko changed the title 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) Nov 16, 2020
@maflcko maflcko merged commit 2fa085a into bitcoin:master Nov 16, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2020
Merge bitcoin#20033: refactor: minor whitespace fixups, s/const/const…
806ac5f 
…expr/ and remove template (followup to bitcoin#19845)

89836a8 style: minor improvements as a followup to bitcoin#19845 (Vasil Dimov)

Pull request description:

  Address suggestions:
  bitcoin#19845 (comment)
  bitcoin#19845 (comment)
  bitcoin#19845 (comment)

ACKs for top commit:
  jonatack:
    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.
  hebasto:
    re-ACK 89836a8
  theStack:
    ACK 89836a8

Tree-SHA512: 36477fdccabe5a8ad91fbabb4655cc363a3a7ca237a98ae6dd4a9fae4a4113762040f864d4ca13a47d081f7d16e5bd487edbfb61ab50a37e4a0424e9bec30b24
@Fuzzbawls Fuzzbawls mentioned this pull request Aug 12, 2021
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Aug 12, 2021
@furszy
Merge #2504: [Refactor] Clang-tidy and fix compiler error in HasReaso…
51055cb 
…n class

03525ea [Refactor] Clang-tidy and fix compiler error in HasReason class (Fuzzbawls)

Pull request description:

  Came across this compiler error when updating the [code coverage report](https://www.fuzzbawls.pw/pivx/regression-test-coverage/index.html). The server runs on CentOS 7 currently, which uses a fairly old (but still supported) version of gcc. Indeed, upstream also ran into this error (Ref: bitcoin#20033)

  Some older (but still supported) versions of gcc will throw an error
  when trying to convert from `std::ios_base::failure` to `std::runtime_error`.
  Use `std::exception` base type instead.

  Also apply clang-format to the class and remove the `cout`.

ACKs for top commit:
  random-zebra:
    utACK 03525ea
  furszy:
    utACK 03525ea and merging..

Tree-SHA512: 46a92f96d30c5f0f29babc30841b0b8b694fabe9ee0e8a5ed79ebc1e405c8718515b718ef12384973b0b003adb907378b27c1fca7f00581e7c3a742d59e9f9ba
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 23, 2021
@vasild @PiRK
refactor: use constexpr and remove template
df99eb6 
Summary:
Minor follow-up to [[bitcoin/bitcoin#19845 | core#19845]] (D10720).

The template was thought to be needed to handle both `std::ios_base::failure` and `std::runtime_error`, but they both inherit `std:eexception` so it is not necessary.

This is a backport of [[bitcoin/bitcoin#20033 | core#20033]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10732
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@jonatack jonatack jonatack left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vasild @hebasto @ajtowns @fanquake @jonatack @theStack @maflcko