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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
