[Lldb-commits] [PATCH] D143520: Add a new SBDebugger::SetDestroyCallback() API

[Greg Clayton via Phabricator via lldb-commits]

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