Skip to content

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Jul 3, 2024

base_tester used to have an empty virtual destructor: virtual ~base_tester() {}. Having a destructor disables the implicit move constructor.

My savanna_cluster has tester as a base class of its node. It is convenient because we can call all the tester methods directly.

However, in a recent change as a PR review request, I updated the nodes from the savanna_cluster to be stored in a std::vector instead of a std::array. std::vector requires its elements to have a default and a move constructor.

To address this issue, in the recently merged PR, I removed virtual ~base_tester() {}, which has the side effect of re-enabling the default move constructor, so savanna_cluster compiled fine. However it created compilation warnings in other tests such as:

In file included from /home/greg/github/enf/spring/plugins/producer_plugin/test/test_trx_full.cpp:5:
/home/greg/github/enf/spring/libraries/testing/include/eosio/testing/tester.hpp:564:7: error: constructor for 'eosio::testing::tester' must explicitly initialize the base class 'base_tester' which does not have a default constructor
  564 |       tester(setup_policy policy = setup_policy::full, db_read_mode read_mode = db_read_mode::HEAD, std::optional<uint32_t> genesis_max_inline_action_size = std::optional<uint32_t>{}) {
      |       ^

This PR adds back the explicit empty destructor in base_tester, as well as the two required defaulted constructors. Everything now compiles without tester-related warnings.

@greg7mdp greg7mdp requested review from heifner and spoonincode July 3, 2024 14:55
@greg7mdp greg7mdp merged commit 7dd41a1 into main Jul 3, 2024
@greg7mdp greg7mdp deleted the fix_warning branch July 3, 2024 15:52
@ericpassmore
Copy link
Contributor

Note:start
group: CLEANCODE
category: INTERNALS
summary: Put back empty base_tester destructor for better hygiene.
Note:end

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