Consistently mark enumerant names #779
Conversation
There was a problem hiding this comment.
Mostly LGTM, though I'd like to see the warning in the spec Makefile cleaned up before merging.
We should also be ready just in case these changes break somebody's workflow - the XML file tends to be used in surprising ways.
Thanks for fixing all of these! This is a nice improvement.
| <enum value="0x12A8" name="CL_COMMAND_COMMAND_BUFFER_KHR"/> | ||
| </enums> | ||
|
|
||
| <enums start="0x12A9" end="0x12B1" name="cl_device_info" vendor="Khronos"> |
There was a problem hiding this comment.
This change cause one of the scripts ("gencl") executed by the spec Makefile to start to complain:
scripts/gencl.py -registry xml/cl.xml -o /home/bashbaug/git/OpenCL-Docs/generated/api apiinc
WARNING: Attempt to redefine cl_command_type (this should not happen)
WARNING: Attempt to redefine cl_device_info (this should not happen)
WARNING: Attempt to redefine cl_kernel_sub_group_info (this should not happen)I'm not sure if the best way to fix this is in the script or in the XML file, but I bet @oddhack knows...
There was a problem hiding this comment.
Probably best to suppress the warning in the script if there is no 'type' attribute on the <enums> tag. If there's a 'type' attribute, then the <enums> are defining a derived type with that name, rather than the name simply being a label on a bunch of constants. If you want I'll propose an MR to that effect.
| <enum value="0x2033" name="CL_KERNEL_MAX_SUB_GROUP_SIZE_FOR_NDRANGE"/> | ||
| <enum value="0x2033" name="CL_KERNEL_MAX_SUB_GROUP_SIZE_FOR_NDRANGE_KHR"/> | ||
| <enum value="0x2034" name="CL_KERNEL_SUB_GROUP_COUNT_FOR_NDRANGE"/> | ||
| <enum value="0x2034" name="CL_KERNEL_SUB_GROUP_COUNT_FOR_NDRANGE_KHR"/> | ||
| </enums> | ||
|
|
||
| <enums start="0x2035" end="0x201F" name="enums.2035" vendor="Khronos" comment="Reserved for interop with other APIs"> |
There was a problem hiding this comment.
This is no worse than it was previously, but not all of these enums are used for "interop with other APIs".
I'm fine addressing this with a subsequent PR if you'd like, but please file an issue referencing this comment so we don't forget about it. Thanks!
There was a problem hiding this comment.
the comment is obviously wrong but changing everything at once is usually not the best way to get PRs accepted :)
There was a problem hiding this comment.
This is just historical hangover - when the block was initially reserved that was its purpose. Sometimes it's useful to actually keep related enums contiguous and in those cases actually reserving a block specific to that purpose makes sense, but it doesn't here and now.
Tokens from the main block are marked rather inconsistently - some are separated in their own "enums" elements, but many are not.
This adds some of the missing enums.