[Lldb-commits] [PATCH] D93481: [lldb/Lua] add support for multiline scripted breakpoints

[Pedro Tammela via Phabricator via lldb-commits]

tammela added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Lua/Lua.h:39
   llvm::Error LoadModule(llvm::StringRef filename);
+  llvm::Error LoadBuffer(llvm::StringRef buffer, bool pop_result = true);
   llvm::Error ChangeIO(FILE *out, FILE *err);
----------------
labath wrote:
> tammela wrote:
> > labath wrote:
> > > This default value does not seem particularly useful. In fact, I am not sure if this argument should even exist at all. The usage in `IOHandlerIsInputComplete` is sufficiently unorthodox that it might be a good idea to draw attention to the fact that something funky is happening via an explicit pop (and maybe a comment).
> > We can't explicitly pop, as in calling `lua_pop()`, from `ScriptInterpreterLua.cpp` because the `lua_State` is not exposed. Are you suggesting to add a method that wraps `lua_pop()` as well?
> Good question. I didn't realize this aspect of the problem. I think the answer to this depends on the direction we want the Lua class to go in. Back when it was first introduced, my idea was for it to be some sort of a c++ wrapper for the lua (C) api. It would offset a nicer interface to interact with lua, but would otherwise be (more-or-less) lldb-agnostic (kind of like our PythonDataObjects). In that world, it would make sense to expose the lua stack and the pop function (among other things).
> 
> However, I don't think that's really how things have worked out. The current Lua interface is much more high-level and much-more debugger-oriented than I originally imagined. Among functions like `RegisterBreakpointCallback` and `ChangeIO`, a function like `Pop` seems misplaced. I'm not sure this is a good position to be in (there's still a need to find a home for the c++ wrapper-like functions, for example to handle exceptions), but if we want to go down that path, then a pop function is not the right answer. However, I think the same applies to the LoadBuffer function, as it cannot be useful (beyond this syntax-check use case) without exposing the lua stack, implicitly or explicitly.  In this world it would be better to make the function even more high-level, and have something like `CheckSyntax(buffer)` (which does pretty much what LoadBuffer does, but it always pops the result). Internally it can be implemented as a call to LoadBuffer, but this function would now be a private implementation detail instead of a part of the interface.
I think it's better to keep it as high level as possible. Exposing the `lua_State` spills the stack control to whom ever has access to the Lua interpreter, which might be undesirable and cumbersome to keep track of.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93481