-
Notifications
You must be signed in to change notification settings - Fork 258
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
Lua and Python have counter-intuitive naming in Dim table #415
Comments
Note, I am not a Lua expert, or even really a Lua novice! Is it possible to introduce Lua "aliases", so the same values can be referred to by two different names? If so, would it make sense to add aliases for these cases, so old code continues to work while new code could use the new name? I suppose if we did this we could also add aliases for the previous names, so we would consistently have both a name with a "Dim" prefix and without any prefix. |
Aliasing isn't really a thing a Lua, mainly because it tends not to be necessary. In this case, as well as most other cases, you can just assign the same value to multiple names. The way the values are being assigned in this file is as a bunch of "tables" which under the hood are equivalent to hashtables or dictionaries, so it also wouldn't slow anything down to add more stuff. Using the Dim table as an example, this would work fine:
|
FWIW, it seems the table constructor syntax of '[' exp ']' = exp got added in Lua 3 in 1997, so there should be no real concerns about compatibility with older versions of Lua. Compatibility with existing software using these definitions, as raised by @bashbaug, is another matter though. Do we know which projects might use spirv.lua? My knowledge of Lua has faded quite a bit over the years, but I can imagine it being a problem that code might want to iterate through all of the key+value pairs of Dim, in which case adding the new lines ( Could we have this be configurable, depending upon a global defined before the "require" line for spirv.lua, so that users can choose whether they want old names, new names, or both? FWIW, the code that handles this is in tools/buildHeaders/header.cpp in TPrinterLua and is quite simple. In TPrinterLua we have the following code:
prependIfDigit is adding the "Dim" here. We use it in:
Obviously languages in the C family need this, but Python does not, and I'm not really sure about JSON (it might not require it, but perhaps it might be problematic for some JSON implementations – I don't know). I would suggest that whatever change we might make here, we make for Python too, unless there's a good argument against this. As such, I've updated the title of this issue. |
I'm not sure what the standards are. It certainly seems like the keys for the tables in the Lua version of the SPIR-V main header are supposed to match the keywords in the spec and also as used in the reference front-end's assembly. If that is the case, then having the 1D, 2D and 3D dimensionality keys be appended with Dim is a pitfall that needs to be highlighted. If not, then it'd still be nice if the file's notes at the beginning mentioned that the tokens aren't meant to be used directly.
That said, mainly the reason I wanted to post about this is that, looking at the formatting and such, it really seems like whoever made that header assumed that the keys "1D", "2D" and "3D" aren't valid table keys in Lua.
They are. This is how that table should be written to maintain consistent usage:
The text was updated successfully, but these errors were encountered: