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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks. For posterity, I'm going to summarize my thoughts from that discussion.

I was arguing that "require" is not a good thing to be using here, because it's hard for it to guarantee that it will load a specific file. Now, that does not matter for "normal" uses of "require", but it is pretty unfortunate for the "command script import" command. This command takes an explicit filename argument, and it'd be pretty surprising to have "command script import /bar/foo.lua" load "/baz/foo.lua" just because "/baz" happened to come first in the search path.

Since figuring out what to load is the largest part of the "require" function, and that's exactly the part we don't need here, I think we can just drop "require" and implement the loading ourselves. (The other responsibility of the "require" function is to ensure a module is not loaded twice, but this is something we need to override anyway, as the "command script import" contract states that the module is always reloaded.)

Now back to the current patch: I see that you've dropped the part which assigns the result of evaluating the module to a global variable (and consequently, dropped the "local" keyword from the "mymodule" declaration). That seems unfortunate, as the recommended way for writing modules is to make the variable local, and return the module as a result.

What's the reasoning behind that? I was expecting we would keep that part... All it would take is adding an extra `lua_setglobal(m_lua_state, name)` call after the `pcall` stuff.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h:43-46
+  llvm::Expected<std::string> FormatQuoted(llvm::StringRef str);
   std::mutex m_mutex;
   lua_State *m_lua_state;
+  llvm::StringSet<> m_loaded_modules;
----------------
I guess these are not needed anymore...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D71825