Skip to content

Conversation

@DanielGibson
Copy link
Contributor

@DanielGibson DanielGibson commented Apr 9, 2025

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 #1011

@MarkCallow
Copy link
Collaborator

Please rebase against latest main and add an entry to the matrix in .github/workflows/linux.yml to run a build with clang to avoid future problems like this. The build should build tools, tests and loadtests but does not need to run the CTS, I think.

@DanielGibson
Copy link
Contributor Author

DanielGibson commented Apr 14, 2025

rebasing is done, I'll look into adding clang

@DanielGibson
Copy link
Contributor Author

Added Clang support, I hope it works (we'll see after the workflow run has been approved)

@MarkCallow
Copy link
Collaborator

Please add a clang Release build too. You can just change config in the matrix entry to Debug,Release.

@DanielGibson DanielGibson force-pushed the fix-clang-warnings branch 2 times, most recently from b004e03 to 7066356 Compare April 15, 2025 04:44
@DanielGibson
Copy link
Contributor Author

ok, done, also rebased against current main

compiler: clang
vk_sdk_ver: '1.3.290'
options: {
config: Debug, Release,
Copy link
Collaborator

@MarkCallow MarkCallow Apr 15, 2025

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't work either

Copy link
Contributor Author

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

@DanielGibson DanielGibson force-pushed the fix-clang-warnings branch 5 times, most recently from 169c9e6 to 838c3f8 Compare April 16, 2025 13:53
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
@MarkCallow
Copy link
Collaborator

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.

@MarkCallow MarkCallow merged commit fbb5412 into KhronosGroup:main Apr 16, 2025
38 checks passed
@MarkCallow
Copy link
Collaborator

Thank you for your hard work on this.

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.

Clang 18 Warnings in tools/ktx/command_compare.cpp

2 participants