-
Notifications
You must be signed in to change notification settings - Fork 284
Shut up warnings clang with libstdc++ emits about non-virtual destructors #1012
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
Shut up warnings clang with libstdc++ emits about non-virtual destructors #1012
Conversation
0f10f89 to
faf9cae
Compare
|
Please rebase against latest main and add an entry to the matrix in |
faf9cae to
0006dcf
Compare
|
rebasing is done, I'll look into adding clang |
|
Added Clang support, I hope it works (we'll see after the workflow run has been approved) |
|
Please add a clang Release build too. You can just change config in the matrix entry to |
b004e03 to
7066356
Compare
|
ok, done, also rebased against current main |
.github/workflows/linux.yml
Outdated
| compiler: clang | ||
| vk_sdk_ver: '1.3.290' | ||
| options: { | ||
| config: Debug, Release, |
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.
Must be Debug,Release. No spaces and no trailing comma.
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.
Please fix. I want to get this merged.
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.
Unfortunately it is still not working. The env. var. CONFIGURATION is only being set to Debug. I think you need single quotes when setting the matrix value:
config: 'Debug,Release',
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.
doesn't work either
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.
I just duplicated the clang section now, once for Release and once for Debug
169c9e6 to
838c3f8
Compare
It complains a lot about non-final classes (structs) derived from KVEntry having virtual functions but no virtual destructor, which in std::optional code (that explicitly calls the destructor somewhere) could theoretically lead to the wrong destructor being called (that of the struct directly derived from KVEntry instead of the one of a hypothetical class derived from that struct that might actually need its destructor called). This doesn't matter in practice because 1. those classes don't have any implemented constructors that need to do anything 2. no further classes are derived from those that are derived from KVEntry So (I think!) these warnings are only hypothetical, but still annoying. Apparently this does not happen if Clang uses libc++ or MSVCs C++ stdlib (in the ClangCL case), but only with libstdc++, maybe even only with libstdc++ from GCC14, but that's the only version I have on my machine. fixes KhronosGroup#1011
838c3f8 to
1b81738
Compare
|
Sorry about all the troubles. This workflow needs to use the "Ninja Multi-Config" generator for the multi-config builds to work. The separate matrix entries you have just added are good for now. I'll work on converting to "Ninja Multi-Config" later. |
|
Thank you for your hard work on this. |
It complains a lot about non-final classes (structs) derived from KVEntry having virtual functions but no virtual destructor, which in std::optional code (that explicitly calls the destructor somewhere) could theoretically lead to the wrong destructor being called (that of the struct directly derived from KVEntry instead of the one of a hypothetical class derived from that struct that might actually need its destructor called).
This doesn't matter in practice because
So (I think!) these warnings are only hypothetical, but still annoying.
Apparently this does not happen if Clang uses libc++ or MSVCs C++ stdlib (in the ClangCL case), but only with libstdc++, maybe even only with libstdc++ from GCC14, but that's the only version I have on my machine.
Fixes #1011