-
Notifications
You must be signed in to change notification settings - Fork 214
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
fix: add missing destructor for CoreMPL and final
for nested classes
#359
Conversation
There is no actual memory leak because no data in the the CoreMPL But it resolves clang's warning: /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/unique_ptr.h:95:2: warning: delete called on non-final 'bls::CoreMPL' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor] delete __ptr; ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/unique_ptr.h:396:4: note: in instantiation of member function 'std::default_delete<bls::CoreMPL>::operator()' requested here get_deleter()(std::move(__ptr)); ^ main.cpp: note: in instantiation of member function 'std::unique_ptr<bls::CoreMPL>::~unique_ptr' requested here static const std::unique_ptr<bls::CoreMPL> pSchemeMLP{std::make_unique<bls::BasicSchemeMPL>()};
'This PR has been flagged as stale due to no activity for over 60 |
@@ -39,6 +39,7 @@ class CoreMPL { | |||
public: | |||
CoreMPL() = delete; | |||
CoreMPL(const std::string& strId) : strCiphersuiteId(strId) {} | |||
virtual ~CoreMPL() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still necessary when you make the other types final?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because final in other class mean just no more inheritance and it doesn't need its own virtual destructor. But above class/struct still needs so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. I guess we already have other virtual functions in this class, and we do use it polymorphically. so this seems right.
Hi @knst, our branch protection rules require that all commits be signed before merging. If you know how to retroactively sign your initial commit, we can proceed with this PR. If you aren't sure how to do that, I've also prepared this PR #371 to merge in, with your attribution listed as co-author. Please let me know how you'd like me to continue 👍 |
…hia-Network#371) Signing commit from @knst from Chia-Network#359 Co-authored-by: Konstantin Akimov <545784+knst@users.noreply.github.com>
…hia-Network#371) Signing commit from @knst from Chia-Network#359 Co-authored-by: Konstantin Akimov <545784+knst@users.noreply.github.com>
There is no actual memory leak because no data in the the CoreMPL
But it resolves clang's warning: