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

Address feedback on enum validation #2087

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tobski
Copy link
Contributor

@Tobski Tobski commented Mar 20, 2023

Fixes #2077

CC @krOoze

@Tobski
Copy link
Contributor Author

Tobski commented Mar 20, 2023

@oddhack are the CIs failing here also failing on main? I can't see any reason for them to fail just from the commits here 🤔

@ShabbyX
Copy link
Contributor

ShabbyX commented Mar 20, 2023

@oddhack are the CIs failing here also failing on main? I can't see any reason for them to fail just from the commits here thinking

I see the same failures on another PR

@oddhack
Copy link
Contributor

oddhack commented Mar 20, 2023

@oddhack are the CIs failing here also failing on main? I can't see any reason for them to fail just from the commits here thinking

Yes, there were some changes in Vulkan-Hpp in response to recent XML schema changes that aren't reflected here yet. Hopefully #2088 will fix that although Actions seems to be running terribly slowly at the moment.

@oddhack
Copy link
Contributor

oddhack commented Mar 20, 2023

@Tobski if you rebase on main branch it should pass CI now. The Rust bindings are still not working but they're not a blocker.

@oddhack oddhack requested review from fluppeteer and r-potter March 22, 2023 13:41
type.
Use of an enumerant is valid if the following conditions are true:
An enumerant value is valid for a particular use if the following conditions
are true:
Copy link
Contributor

@krOoze krOoze Mar 26, 2023

Choose a reason for hiding this comment

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

maybe "all the following conditions".

Choose a reason for hiding this comment

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

We're inconsistent about whether we use "all". I agree it wouldn't hurt!

@@ -233,6 +233,9 @@ include::{generated}/api/protos/vkEnumerateInstanceVersion.adoc[]
of Vulkan supported by instance-level functionality, encoded as
described in <<extendingvulkan-coreversions-versionnumbers>>.

If flink:vkGetInstanceProcAddr returns a code:NULL pointer when this
Copy link
Contributor

Choose a reason for hiding this comment

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

I think styleguide uses only ticks for NULL

object as its first parameter and either:
for the current instance.
* If the enumerant is used in a function that has a dispatchable object
created or allocated from a device as its first parameter and either:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect. Either "dispatchable object created or allocated from a physical device" or "dispatchable object that is device or its children".

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.

Enumerant validity rules seem incorrect
5 participants