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

Fuzzer improvements (janus branch) #58

Conversation

slowriot
Copy link
Contributor

@slowriot slowriot commented Sep 1, 2021

Description

Warnings I added in #54 and #56 highlight certain improvements to make to the newly added fuzzer on the janus/release-0.1.7 branch; the following changes silence these warnings and provide the following improvements:

  • Added c++17 attributes to mark unused function parameters, improving clarity and making it easier to spot if parameters are unintentionally unused.
  • Unused argv and argc parameters removed from fuzzer's main().
  • Remove unused variable declaration in util_tests.
  • Avoid implicit conversions that alter a value (float promotion to double), make those explicit conversions instead, or compare floats to float literals rather than floats to double literals.
  • A copy assignment operator never returns in debug mode, as it contains an assert(false ...) - in this case, the function is now marked [[noreturn]] to clarify expectations. The return call is now only present in the absence of the NDEBUG macro, where the assert would have no effect.
  • CAddrMan had a non-virtual destructor, despite being a polymorphic base class - the d'tor is now marked virtual as it should be.

BTC/BGL PR reward address

ETH/USDT: 0x50b92AB67A3D3DE8c3506D9287893D9a52655486

@slowriot slowriot mentioned this pull request Sep 6, 2021
@slowriot
Copy link
Contributor Author

slowriot commented Sep 7, 2021

Which value do we gain from this?

The value you gain is as follows:

  • Unintentionally unused function parameters now generate a warning, allowing future developers to catch unintended bugs where an important parameter to a function has been missed.
  • Intentionally unused function parameters are clearly marked as unused, preventing future developers from wondering if there is a bug in the code where the decision has been made intentionally.
  • Removing unused arguments to functions cleans up the code, reduces code size, and improves readability, ultimately reducing ongoing cost of maintenance and technical debt.
  • Implicit conversions between float and double can change the value being converted; this can lead to subtle and unexpected bugs, especially when it the floating point values are compared directly without a sigma. Ensuring there are no implicit conversions avoids potentially difficult to identify and machine-specific bugs.
  • Where a conversion is intentional, making it explicit makes the intention clear to future maintainers, reducing future maintenance burden and technical debt.
  • Introducing a warning to catch unintentional conversions prevents future introduction of such bugs.
  • CAddrMan had a non-virtual destructor, despite being a base class from which others inherit. Base class destructors should be virtual, otherwise memory leaks can result - this is a common bug and there is no question whether it is correct for the destructor to be made virtual. It absolutely should be, in this case. That is why GCC produces a warning for this code, and my changes also add a warning to catch other such bugs.

The rest of your comments do not appear to be associated with specific lines of code in the Github view, so it is difficult for me to answer them specifically; if you clarify what line of code you are referring to with each, I can reply in more detail.

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.

3 participants