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

Fix for VULKAN_HPP_TYPESAFE_CONVERSION defined to 0 #1806

Merged

Conversation

d235j
Copy link
Contributor

@d235j d235j commented Feb 18, 2024

This fixes #1801. Headers will have to be regenerated/copied.

@asuessenbach
Copy link
Contributor

I think, you don't need to check if VULKAN_HPP_TYPESAFE_CONVERSION is defined and compare it with 1. The pure comparison should suffice, as undefined values are considered to be 0.
Besides that, would be great, if you could add the generated headers as well, as the CI would then check if all (used) compilers agree about the correctness of this statement.

@d235j
Copy link
Contributor Author

d235j commented Feb 20, 2024

I think, you don't need to check if VULKAN_HPP_TYPESAFE_CONVERSION is defined and compare it with 1. The pure comparison should suffice, as undefined values are considered to be 0.

This will trip up -Wundef on some compilers or /wd4668 on MSVC. This warning is not included in -Wall; if it is not a concern then I can change the PR.

Besides that, would be great, if you could add the generated headers as well, as the CI would then check if all (used) compilers agree about the correctness of this statement.

Is there a contribution guide available that indicates what steps should be taken before making a PR? I noticed many whitespace differences between the headers in the repository and those I generated (and I have clang-format installed).

@asuessenbach
Copy link
Contributor

This will trip up -Wundef on some compilers or /wd4668 on MSVC. This warning is not included in -Wall; if it is not a concern then I can change the PR.

I think, those warnings are not a concern here. So please change the PR.

Is there a contribution guide available that indicates what steps should be taken before making a PR? I noticed many whitespace differences between the headers in the repository and those I generated (and I have clang-format installed).

I'm sorry, there's nothing like a contribution guide. Maybe I should introduce one, at some point.
Regarding clang-format: I think, currently clang-format version 14 is used. So, if you could try that one?
And even if there are whitespace differences: would be great to have the generated files in this PR to let the CI check if all (tested) compilers are happy with that change.

Otherwise, if you're ok with that, I could hijack your PR, make it on my own and check the results.

@d235j
Copy link
Contributor Author

d235j commented Feb 21, 2024

Since I left the “Allow edits from maintainers” checked you should be able to commit and push to my PR (this is a newer GitHub feature). Feel free to utilize it. Otherwise I can update the PR later. As for clang-format, I believe I was using the latest released version (17) on macOS.

Might it be possible to implement a CI process where the CI detects if the headers need updating and automatically updates them?

@asuessenbach
Copy link
Contributor

@d235j I've adjusted your PR a little. If you are ok with those changes, I'd merge it.

Might it be possible to implement a CI process where the CI detects if the headers need updating and automatically updates them?

The detection step should be possible, but I don't see/know how to automatically update them.

@asuessenbach asuessenbach merged commit 1722636 into KhronosGroup:main Mar 11, 2024
12 checks passed
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.

VULKAN_HPP_TYPESAFE_CONVERSION cannot be set to 0 to disable implicit conversions
2 participants