Issues

Issue #622 resolved

Thread and Shader construction inconsistency

hahawoo
created an issue

Threads and Shaders are similar in that they are objects created from code in plain text.

However, Threads can be created from files by specifing a file path or a File object and can be created from strings via love.filesystem.newFileData, whereas Shaders can be created from strings directly and from files using love.filesystem.read/File:read.

Should they both use the same style of constructor?

The Thread constructor is consistent with other object constructors (strings for file paths, FileData for data-containing strings), but the Shader constructor is perhaps more natural for things created from plain text.

A possible objection for changing anything could be that Threads are more likely to be in files and Shaders are more likely to be inline strings, but I'm not sure if this is true, I've seen Threads as strings and Shaders as files. And if it is true, is the difference great enough to warrant different styles of construction?

So, my light proposal would be to change newThread to accept a string containing the thread data, instead of a file path. I think this would be cool because it makes the simple and easy thing of creating a thread from a string simple and easy to do. And then, if you want to organise things in such a way that the thread is in another file, love.filesystem.read can be used, just like it can be used for shaders now.

Comments (8)

  1. Alex Szpakowski

    With love.graphics.newShader, it can accept either a filename string or a code string. If the string isn't a file then it tries to load it as code, and if that has syntax issues (due to actually being a misspelled file name, for example) then it produces an error.

    With love.thread.newThread, no error is produced if the code passed to it is bad, partly because the code isn't actually loaded until Thread:start, and partly because the code is run in a separate thread so Thread:getError is used.

    This means that if love.thread.newThread were to accept both filename and code strings, accidentally misnamed filepaths would get treated as a code string and no error would be generated in the main thread, which is bad.

    Considering threads are full separate Lua states which often include their own Lua files, it makes more sense to me to only have a constructor for files than to only have a constructor for a code string.

    You can do love.thread.newThread(love.filesystem.newFileData("code here", "fakefilename")).

  2. hahawoo reporter

    That's really cool that love.graphics.newShader works like that, I did not know that! So, turns out they aren't really inconsistent in the way that I thought after all. And having to use love.filesystem.read to create a thread from a file would be inconsistent with everything, including newShader, and now I also think that assuming threads would be created from files rather than code strings does make sense.

  3. hahawoo reporter

    How about this...

    If a string passed to newThread wasn't a file... and it contained no newlines... and it ended in ".lua"... it could trigger a "file not found" error.

    This might catch 99.9% of cases where the name is misspelt, and in the case where it doesn't pick it up, it will just take longer to realise they've misspelt the filename, or in the crazy case of something creating an in-line one-line thread which ends in a table element with the key "lua" being accessed, I think they'd get the idea from the error message. :P

    And it would be super simple to create threads and it would be consistent with newShader! :D

  4. Bart van Strien

    I'm actually not sure if specifying shaders inline is a good idea, either. Even if we forget the context switch, there's still the issue with unrelated code being in the middle of a file.

  5. Alex Szpakowski

    Literal strings containing Lua code can be passed into love.thread.newThread directly, similar to loadstring and love.graphics.newShader (resolves issue #622.)

    Threads are still very heavyweight objects and should be treated as such, unlike the functions returned by loadstring.

    → <<cset ec25d9221a06>>

  6. Log in to comment