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

Get rid of gcc warning -Wnon-virtual-dtor #313

Conversation

wbangna
Copy link
Contributor

@wbangna wbangna commented Mar 31, 2021

We get warnings when compiling the jsonpath extension:
... has accessible non-virtual destructor [-Wnon-virtual-dtor]

Simply defining a defaulted virtual destructor for the base class binary_operator removes the warning.

@wbangna wbangna force-pushed the fix-warning-non-virtual-destructor branch from a7ec5e9 to 8053b7d Compare April 1, 2021 06:17
@danielaparker
Copy link
Owner

danielaparker commented Apr 3, 2021

Thanks for noting this. Going forward, we'll compile the gcc tests with the -Wnon-virtual-dtor compile flags.

I believe though that the binary_operator specializations don't require a public virtual destructor, because they're never destroyed through a pointer to a base class. Could you try an alternative approach to silence the warning, by making the base destructor non-virtual but protected? i.e.

protected:
~binary_operator() = default;

Thanks,
Daniel

Define the destructor of `binary_operator` as protected
@wbangna wbangna force-pushed the fix-warning-non-virtual-destructor branch from 8053b7d to 6035dac Compare April 6, 2021 07:42
@wbangna
Copy link
Contributor Author

wbangna commented Apr 6, 2021

Thank you for your suggestion. I guess that it's safe to have a non-virtual destructor since the class is in the namespace detail.
I changed the destructor as you suggested and tested it with a small test program.

@danielaparker
Copy link
Owner

Thank you for your suggestion. I guess that it's safe to have a non-virtual destructor since the class is in the namespace detail.
I changed the destructor as you suggested and tested it with a small test program.

Thanks.

@danielaparker danielaparker reopened this Apr 6, 2021
@danielaparker danielaparker merged commit 6565468 into danielaparker:master Apr 6, 2021
@danielaparker
Copy link
Owner

Thanks, merged (after I initially forgot to merge it before closing it!) The main reason for making it non-virtual protected is that it documents and enforces an internal design choice.

@danielaparker
Copy link
Owner

I added -Wnon-virtual-dtor in the build tests, and there a few other cases like that in the jsonschema and jmespath extension. I'll fix those.

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.

None yet

2 participants