KhronosGroup / SPIRV-Headers

SPIRV-Headers
Other
269 stars 254 forks source link

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

Open kimiyoribaka opened 8 months ago

kimiyoribaka commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

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:

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.