Remove "enumerate" functions whitelist #160
Conversation
|
@Snektron Ping, waiting for your thoughts on this whenever you get the chance:
|
|
Ah sorry, I though it was still in progress considering the draft state. Ill get back to you tomorrow |
|
Hi sorry for the delay.
I agree with you, and I guess this is the approach that's already taken for a few other commands. You found quite a nice method and list of functions that satisfy the required criteria. I wonder if there's something that we could do with that, like a script that prints out the candidates? I don't want to create any work for you though, I'm fine leaving the state like this if you think that's whats best. Thanks for taking the time to look into this, and the patience for my slow reviews :) |
|
No problem! First off, I don't want to take up too much of your time with this so if you don't feel like responding in detail then that's totally fine - I'm happy to forget about the following and just throw together a quick script that should be easy to review :)
Yeah I can do something like that, I agree it would be a waste to just throw away the investigation so far. You think a separate script is best, or what about adding a command line argument to the generator to log these functions? The latter wouldn't take much code, although of course it will still add a bit of maintenance overhead - might be worth going the separate route in order to avoid cluttering the generator, I don't feel strongly either way. I haven't looked into this closely yet but a possible different approach, we could try going ahead with this automatic classification for core functions only and use a whitelist for extension functions. Generally the core stuff seems a lot more consistent so we're less likely to run into those edge cases, and most extensions are pretty niche so if we only add things people actually need instead of making it exhaustive, it wouldn't be a long list. There would still be a chance of things breaking in the future, but IMO a much smaller chance than including all extension functions. |
| if (!(count_param.is_buffer_len and data_param.is_optional)) { | ||
| return null; | ||
| } | ||
|
|
||
| // Uses a bool out-parameter for an "incomplete" result, and so would need a | ||
| // special case implementation. | ||
| if (mem.eql(u8, name, "vkGetFaultData")) { | ||
| return null; | ||
| } | ||
|
|
||
| // TODO: Extra validation. | ||
|
|
There was a problem hiding this comment.
Just some thoughts:
- Did you consider checking the mutability of the output parameter? That should be something like
param_type.pointer_is_mutable, or simply useclassifyParamand checkout_pointer(I think). Additionally, you could check that the size param has classmut_buffer_len - Did you consider checking whether the successcodes includes
VK_INCOMPLETE?
Maybe this would be enough and then we don't need to bother with the scripts and stuff
Closes #148
This seems to correctly classify existing functions (although I need to check more carefully before merging) but I'm trying to add some redundant checks for future-proofing and am running into some annoying inconsistencies in a few of them.
E.g. there's
vkGetFaultData()as mentioned in the diff, andvkGetPipelineBinaryDataKHR()which takes an extra out-parameter and returnsVK_ERROR_NOT_ENOUGH_SPACE_KHRinstead ofVK_INCOMPLETE. Side note, I should maybe blacklist that one: the generated code is correct, but it relies in a kind of fragile way on the generator's handling of out-parameters vs. inout-parameters. And those two are just from a quick spot-check, there are likely more I haven't noticed.IMO it's maybe worth forgetting about this and just leaving the whitelist. It's not necessarily pretty but is probably more maintainable than dealing with these special cases and risking that the generator will break with future spec versions and/or some of these functions will be wrong in weird ways. But I don't want to leave things in a state you're unhappy with, so if you really want to get rid of the whitelist then let me know and I can go ahead with this :)