Skip to content
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.

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

[Compilation] Pass caught exceptions by reference #979

Merged
merged 3 commits into from
Sep 21, 2019

Conversation

Warrows
Copy link

@Warrows Warrows commented Aug 6, 2019

Gets rid of compiler warnings such as "warning: catching polymorphic type 'class std::runtime_error' by value [-Wcatch-value=]"

@Warrows Warrows self-assigned this Aug 6, 2019
@CaveSpectre11
Copy link

CaveSpectre11 commented Aug 6, 2019

Some questions:

  • The format is consistent in these changes, but it's not consistent through the code base. Should the format be <datatype>& <identifier> or <datatype> &<identifier>?
  • without an identifier; some places in the code are by reference, some by value
    test\alert_tests\cpp Line 96: catch (std::exception) { }
    rpc\rawtransaction.cpp Line 620: } catch (const std::exception&) {
  • While we're at it; should we define the catches to be const?
  • Should this be expanded to include the test modules?
    test\benchmark_zerocoin.cpp
    test\libzerocoin_tests.cpp
    test\alert_tests.cpp

@Warrows Warrows force-pushed the 2019-08_exceptions-by-ref branch 2 times, most recently from 1954e3d to 3b61e81 Compare August 7, 2019 12:19
@Warrows
Copy link
Author

Warrows commented Aug 7, 2019

Addressing your questions:

  • Well that's my bad because https://github.com/PIVX-Project/PIVX/blob/master/doc/developer-notes.md#coding-style-c specifies that the desired format is <datatype>& <identifier>. I fixed it.
  • I fixed the warnings thrown by my compiler. Here is an explanation about the reasons behind this warning: https://blog.knatten.org/2010/04/02/always-catch-exceptions-by-reference/ We can assume that the places such as test\alert_tests\cpp Line 96: catch (std::exception) { } don't throw a warning because the exception is not used, thus we don't really care if it's missing some specific methods or data. I fixed it anyway so it can be used safely in the future.
  • Indeed, adding const to exceptions catching is not that important of a change but it's always good to have. So I added it too.
  • Of course test modules must be included. And they were. Only alert_tests.cpp wasn't changed because as I said above it didn't throw a warning. That's done too now.

@CaveSpectre11
Copy link

Good reference; I will check it out so I can be more authoritative in the future ;)

@Warrows Warrows force-pushed the 2019-08_exceptions-by-ref branch 2 times, most recently from 05e8d12 to 5780e0d Compare August 14, 2019 09:58
@Warrows Warrows added this to the 4.0.0 milestone Aug 14, 2019
random-zebra
random-zebra previously approved these changes Sep 16, 2019
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK 5780e0d

@random-zebra
Copy link

Needs rebase

Gets rid of compiler warnings such as "warning: catching polymorphic type 'class std::runtime_error' by value [-Wcatch-value=]"
@Warrows
Copy link
Author

Warrows commented Sep 19, 2019

Rebased

@random-zebra
Copy link

Doesn't compile. There's some conflict leftover in masternode.cpp since #1001 has been merged.

@Warrows
Copy link
Author

Warrows commented Sep 19, 2019

Indeed. Should be good now.

@furszy furszy self-requested a review September 20, 2019 21:27
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK c826092

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK c826092

random-zebra added a commit that referenced this pull request Sep 21, 2019
c826092 [Refactor] Replace tabs with spaces (warrows)
b69f37e [Refactor] Add const qualifier to exception catching (warrows)
bf91dac [Compilation] Pass caught exceptions by reference (warrows)

Pull request description:

  Gets rid of compiler warnings such as "warning: catching polymorphic type 'class std::runtime_error' by value [-Wcatch-value=]"

ACKs for top commit:
  furszy:
    ACK [`c826092`](c826092)
  random-zebra:
    ACK c826092

Tree-SHA512: a2c02fdeab2d0e3cddde7a983456a680776592e9381be90968e0dc04bc86b7d5ea4d9aeb8259d39060fcd04583bb35259170fe5c28b8f0e96cf7aa94a3f21b2c
@random-zebra random-zebra merged commit c826092 into PIVX-Project:master Sep 21, 2019
@Warrows Warrows deleted the 2019-08_exceptions-by-ref branch September 22, 2019 20:24
Fuzzbawls added a commit that referenced this pull request Dec 21, 2020
… in CreateSig

2523f81 [Refactor] Pass caught logic_error by reference in CreateSig (random-zebra)

Pull request description:

  Exceptions/errors caught should be passed by reference.
  If the catch-block is not modifying the exception/error, the reference should be `const` (same as we did way back in #979)

  Fix one instance in `TransactionSignatureCreator::CreateSig` where we are passing a logic_error by copy, causing the `catching polymorphic type` warning reported in #2076

ACKs for top commit:
  furszy:
    utACK 2523f81
  Fuzzbawls:
    utACK 2523f81

Tree-SHA512: d13a0a2a9372610f3a5e67016053afbbac94d30f5d3dd8568352f7fd99356336058405f8526eaf1e6f751e2e48d7405425ad7a8a3a3f0ba8c6db620d12710ad2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants