[Lldb-commits] [PATCH] D71825: [lldb/Lua] Support loading Lua modules.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 5 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:49
+    if (llvm::Error e =
+            Run(llvm::formatv("package.path = package.path .. \";{0}\"",
+                              file.GetCString())
----------------
labath wrote:
> Embedding the file name this way seems unfortunate (`"; os.execute("rm -rf /");`), and it probably will not work at all on windows because of all the backslashes. Lua formatting function has a `%q` format to format strings safely. Accessing that from C is a bit tricky, but we can make a utility function for that.
I've used Lua's %q implementation. Calling string.format() didn't work for the require statement. 


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.cpp:66
+  return Run(
+      llvm::formatv("{0} = require \"{0}\"", module_name.GetStringRef()).str());
+}
----------------
labath wrote:
> Is there any advantage to using `require` here? It doesn't seems like it brings much into the game, as its main purpose seems to be to _find_ the module, but here we know exactly where the module is. OTOH, using it has a lot of downsides (needing to quote/validate all strings, mess with the `package.loaded` variable, potentially loading the wrong file, etc).
> 
> Would it be possible to just `luaL_loadfile` the file and then assign the result of its evaluation to the appropriate variable manually?
The advantage is that it's the standard way to import modules in Lua and users should be able to rely on that. If you use `luaL_loadfile` it will circumvent the runtime. This can have undesirable side effects, for example importing the same module again will reload it (which will not happen if it has already been loaded with `require`). 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71825/new/

https://reviews.llvm.org/D71825