fix: drop virtual specifier for CZMQNotificationInterface dtor#7317
Conversation
```
In file included from zmq/zmqnotificationinterface.cpp:5:
./zmq/zmqnotificationinterface.h:22:13: error: virtual method '~CZMQNotificationInterface' is inside a 'final' class and can never be overridden [-Werror,-Wunnecessary-virtual-specifier]
22 | virtual ~CZMQNotificationInterface();
| ^
1 error generated.
```
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 8714052) |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Trivial single-line warning fix removing redundant virtual from ~CZMQNotificationInterface() since the class is final. Verified the patch matches the worktree at 8714052: the change is limited to src/zmq/zmqnotificationinterface.h and is correct — CValidationInterface has a protected non-virtual defaulted destructor, so polymorphic deletion through the base is not intended. Both agents reported no findings; concurring with clean approval.
Reviewed commit: 8714052
…face` dtor 8714052 fix: drop `virtual` specifier for `CZMQNotificationInterface` dtor (UdjinM6) Pull request description: ## Issue being fixed or feature implemented ``` In file included from zmq/zmqnotificationinterface.cpp:5: ./zmq/zmqnotificationinterface.h:22:13: error: virtual method '~CZMQNotificationInterface' is inside a 'final' class and can never be overridden [-Werror,-Wunnecessary-virtual-specifier] 22 | virtual ~CZMQNotificationInterface(); | ^ 1 error generated. ``` ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 8714052 Tree-SHA512: 9026c7902b7cc7db51925e97212cf54ef98aca98d862911cbdc85cc9828bec07163e9b7258b91d1af4752c2df7962ec85f22933abcf13337506f3b6a78ea279d
01640b9 chore: prepare v23.1.5 release (PastaClaw) c3e3b9d fix: correct v23.1.4 checkpoint hash (PastaClaw) 10b6309 docs: use standard bug fixes wording (PastaClaw) 0495b47 docs: refine v23.1.4 release intro (PastaClaw) 3826688 docs: fix v23.1.4 release notes title (PastaClaw) 2068176 Merge #7317: fix: drop `virtual` specifier for `CZMQNotificationInterface` dtor (pasta) 9a463a2 test: early bail-out for huge QSIGSHARESINV and QGETSIGSHARES (Konstantin Akimov) 3426458 fix: early bail-out for huge QSIGSHARESINV and QGETSIGSHARES (Konstantin Akimov) b9c6b95 chore: version bump, man pages, flatpak, release notes for release v23.1.4 (Konstantin Akimov) 6f7a0ef chore: version bump, man pages, flatpak, release notes for release v23.1.4 (Konstantin Akimov) 78bb4c4 Merge #7359: fix: make huge asset lock tx non-standard (pasta) 5962016 chore: update min-chainwork, checkpoints and seeds for v23.1.4 (Konstantin Akimov) 99305fb test: add test for duplicated platform-node-id in mempool (Konstantin Akimov) 78b8274 fix: don't let 2 protx with the same platform-id to be presented in mempool (Konstantin Akimov) 54187cd perf: avoid re-validation of ehf signals during block-connect (Konstantin Akimov) 47fa631 Merge #7352: perf: optimize division to uint32 to make re-target-pow calculation much faster (pasta) d93f755 Merge #7332: fix: remove cs_main from MaybePunishNodeForTx, missing changes from bitcoin#19607 (pasta) Pull request description: # chore: merge master 23.1.5 back into develop ## Issue being fixed or feature implemented Backmerge current `master` into `develop` so the active development branch includes the `v23.1.4` and `v23.1.5` release-line changes. ## What was done? Merged `upstream/master` (`v23.1.5`, `5dde61050d20c23c3e179c34e663a025f56843f3`) into `upstream/develop`. Carried forward release metadata and generated artifacts for `v23.1.4` and `v23.1.5`, including version metadata, man pages, release notes, checkpoints, chainTxData, minimum chain work, assume-valid data, and seed updates. Resolved conflicts in Dash-specific code while preserving develop-only refactors: - Kept develop's `CChainstateHelper` ownership layout and `SuperblockManager` integration. - Carried forward the EHF block-connect optimization without reintroducing the removed `CMNHFManager` quorum-manager member. - Carried forward the LLMQ signing-share inventory bounds into develop's `NetSigning` split. - Carried forward the Platform node ID mempool conflict handling and its functional test coverage. ## How Has This Been Tested? - Ran `git diff --check upstream/develop...HEAD`. - Scanned changed files for conflict markers. - Verified the corrected `CMNHFManager` constructor call after conflict resolution. - Ran the required pre-PR `code-review` gate against the backmerge branch; final recommendation was `ship` with no significant issues found. No local build was run; this is a release backmerge and CI should run the full Dash Core PR build/test matrix. ## Breaking Changes None expected. This is a backmerge from the release branch into `develop`. ## Checklist - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 1a828c9acd5fe87617f3ab11ca1b5b13656adc61e591b58d68b685bba7404696afa664c53cc752dfbaca7fe08439d9428f33116f475c18c0959293460062b284
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist: