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

Lua and Python have counter-intuitive naming in Dim table #415

Open
kimiyoribaka opened this issue Feb 2, 2024 · 3 comments
Open

Lua and Python have counter-intuitive naming in Dim table #415

kimiyoribaka opened this issue Feb 2, 2024 · 3 comments

Comments

@kimiyoribaka
Copy link

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:

    Dim = {
        ["1D"] = 0,
        ["2D"] = 1,
        ["3D"] = 2,
        Cube = 3,
        Rect = 4,
        Buffer = 5,
        SubpassData = 6,
        TileImageDataEXT = 4173,
    },
@bashbaug
Copy link
Contributor

bashbaug commented Feb 9, 2024

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.

@kimiyoribaka
Copy link
Author

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:

    Dim = {
        ["1D"] = 0,
        ["2D"] = 1,
        ["3D"] = 2,
        Cube = 3,
        Rect = 4,
        Buffer = 5,
        SubpassData = 6,
        TileImageDataEXT = 4173,
        Dim1D = 0,
        Dim2D = 1,
        Dim3D = 2,
        DimCube = 3,
        DimRect = 4,
        DimBuffer = 5,
        DimSubpassData = 6,
        DimTileImageDataEXT = 4173,
    },

@StuartDBrady StuartDBrady changed the title Lua header's Dim table is counter-intuitive Lua and Python have counter-intuitive naming in Dim table Feb 22, 2024
@StuartDBrady
Copy link
Contributor

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 (["1D"] = 0, etc.) without removing the old lines (Dim1D = 0, etc.), as the same values (0, 1 and 2) would be seen twice.

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:

        std::string enumFmt(const std::string& s, const valpair_t& v,
                            enumStyle_t style, bool isLast) const override {
            return indent(2) + prependIfDigit(s, v.second) + " = " + fmtStyleVal(v.first, style) + ",\n";
        }

prependIfDigit is adding the "Dim" here. We use it in:

  • TPrinterJSON::enumFmt
  • TPrinterCPP11::enumFmt
  • TPrinterLua::enumFmt
  • TPrinterPython::enumFmt
  • TPrinterCSharp::enumFmt
  • TPrinterD::enumFmt
  • TPrinterBeef::enumFmt

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.

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

No branches or pull requests

3 participants