clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed.
If I am reading the code for this patch correctly, we need the BatonSP stuff because we have differing callback bytes for public vs private APIs. If we switch to using a common callback type of: typedef void (*DebuggerDestroyCallback)(lldb::user_id_t &debugger_id, void *baton); (we can define this both internally and leave the current "SBDebuggerDestroyCallback" in SBDefines.h alone), then we can avoid having to add any of the BatonSP stuff and just store a "void * m_destroy_callback_baton;" in Debugger.h. ================ Comment at: lldb/include/lldb/Core/Debugger.h:598-599 + lldb::DebuggerDestroyCallback m_destroy_callback = nullptr; + lldb::BatonSP m_destroy_callback_baton_sp; + ---------------- Why do we need the fancy BatonSP stuff here? Can't we just store this as a void *? Or is the fancy baton stuff required for the python for some reason? I couldn't see any reason by reading the code quickly, please correct me if so. ================ Comment at: lldb/include/lldb/lldb-types.h:71-72 typedef void (*LogOutputCallback)(const char *, void *baton); +typedef void (*DebuggerDestroyCallback)(lldb_private::Debugger &debugger, + void *baton); typedef bool (*CommandOverrideCallback)(void *baton, const char **argv); ---------------- This can't be in the public lldb-types.h header file as no on will be able to use it since it uses "lldb_private::Debugger". If this is to be in the public header files, then this needs to change to be one of: ``` typedef void (*DebuggerDestroyCallback)(lldb::user_id_t &debugger_id, void *baton); typedef void (*DebuggerDestroyCallback)(lldb::SBDebugger &debugger, void *baton); ``` The "debugger_id" is the easier one to do since we need to make this work with python so that python users can install a callback from python and have it all work. This can however exist in the lldb-private-types.h which is not part of the public API, so it can be moved there and then it won't cause any issues. I realize there are other typedefs in here than mention lldb_private stuff, but they shouldn't be here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143520/new/ https://reviews.llvm.org/D143520 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
