[Lldb-commits] [PATCH] D153735: [lldb][TargetGetModuleCallback] Implement Python interface

[Kazuki Sakamoto via Phabricator via lldb-commits]

splhack added inline comments.


================
Comment at: lldb/include/lldb/API/SBDefines.h:129
 
+typedef SBError (*SBTargetGetModuleCallback)(SBDebugger debugger,
+                                             SBModuleSpec &module_spec,
----------------
clayborg wrote:
> If we are putting this into SBPlatform, this should probably be named "SBPlatformGetModuleCallback". 
> 
> The name might be a bit more clear if it was "SBPlatformLocateModuleCallback"? Open to suggestions or fine to lave this as "SBPlatformGetModuleCallback" if everyone likes that.
As commented on D153734, I'll rename it to `SBPlatformLocateModuleCallback`.

```
typedef SBError (*SBPlatformLocateModuleCallback)(
    SBDebugger debugger,
    SBModuleSpec &module_spec,
    SBFileSpec &module_file_spec,
    SBFileSpec &symbol_file_spec);
```


================
Comment at: lldb/include/lldb/API/SBPlatform.h:181
+  /// nullptr or None is set.
+  SBError SetTargetGetModuleCallback(lldb::SBTargetGetModuleCallback callback,
+                                     void *callback_baton);
----------------
clayborg wrote:
> remove "Target" from the function name? Or rename to "SetLocateModuleCallback(...)" if we end up renaming the callback type to SBPlatformLocateModuleCallback
I'll rename it to `SetLocateModuleCallback`.
```
SBError SetLocateModuleCallback(lldb::SBPlatformLocateModuleCallback callback,
                                                       void *callback_baton);
```


================
Comment at: lldb/source/API/SBPlatform.cpp:668
+    // Platform. 'callback_baton' is the actual callback Python callable object.
+    platform_sp->SetTargetGetModuleCallback(callback_baton);
+    return SBError();
----------------
clayborg wrote:
> You are not passing the "callback" into Platform::SetTargetGetModuleCallback??
> 
> Are we missing changes to the file "lldb/include/lldb/Target/Platform.h"? 
will update it to support C callback function as well as Python.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153735