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