[Lldb-commits] [PATCH] D120100: [lldb/bindings] Expose the progress reporting machinery to the SWIG interface

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D120100#3332433 <https://reviews.llvm.org/D120100#3332433>, @JDevlieghere wrote:

> In D120100#3331656 <https://reviews.llvm.org/D120100#3331656>, @labath wrote:
>
>> I'm not sure how we ended up with the .i files in the first place, but maybe a good solution to this problem would be to ditch those and generate bindings from .h files directly.
>>
>> It may mean that we need to put `#ifndef SWIG` around some code, but I'd say that beats maintaining two separate copies of the interface. (Also, the ifdefs are a pretty good way to document the bits of the "public" API that we do not wish to make available to scripts.)
>
> I've considered that in the past, but with the docstrings and the inline python code for things like property definitions, I'm not sure it's worth it. I assume we could move the majority of those out of the actual header and do a textual include, but then you're still on the hook for keeping that file in sync.

Yeah, I forgot about how much code we have there. Putting the docstrings into a separate file is fairly easy, but maybe it would actually be fine to put it into the main file directly -- it is still a documentation of what those methods are supposed to do, even if the syntax is funny. And it would avoid the current situation, where some of our SB methods have (doxygen) documentation in the .h files, and others have swig documentation in .i.

But even in a separate, I think it would be better since in would be "just" the documentation, at least we wouldn't have to maintain two copies of the c++ interfaces.



================
Comment at: lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py:22-23
+        listener = lldb.SBListener("lldb.progress.listener")
+        listener.StartListeningForEvents(test_broadcaster,
+                                         self.eBroadcastBitStopProgressThread)
+
----------------
There is a race here, where some (or even all) of the progress events will be generated before the listener actually starts listening for them. You need to ensure that the listener setup happens-before the events are generated. The easiest way to achieve that is to set up the listening on the main thread.


================
Comment at: lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py:38
+                elif event.BroadcasterMatchesRef(progress_broadcaster):
+                    message = lldb.SBDebugger().GetProgressFromEvent(event, 0, 0, 0, False);
+                    if message:
----------------
will anyone be able to read the results provided through the by-ref arguments? That seems like it would be worth testing.

It might also be worth adapting this interface to be more pythonic (having the python wrapper return a tuple instead of by-ref args).


================
Comment at: lldb/test/API/functionalities/progress_reporting/TestProgressReporting.py:42
+
+    @skipUnlessDarwin
+    def test_dwarf_symbol_loading_progress_report(self):
----------------
Why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120100