[Lldb-commits] [PATCH] D75750: WIP: [lldb] integrate debuginfod

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1929124 <https://reviews.llvm.org/D75750#1929124>, @kwk wrote:

> @labath I've updated my patch and would love to hear your opinion on it. So far I've only written the python `ServeDirectoryWithHTTP()` function with proper doctest and documentation but since you mentioned the `0` port thingy I've tried that on the command line when using `python -m http.server 0` and it works smoothly. That's why I've included the `llvm-lit` test I was working on.


Being able to use 0 to auto-assign a port is definitely a big improvement, but there still the question of retrieving that port and sychronization that goes with it, which you've now done with a while loop + sniffing through the server log.

And I still haven't gotten used to how the comments in your lit tests are way longer than the test itself. If I think about that harder, I guess the thing that really bothers me about that (even though I normally like comments) is that there is no visual distinction between "comments" and "code" this way -- it all shows up as grey in the review tool. Can't say that's really your fault, but it does make it hard to see what that test is doing nonetheless. (Some areas of llvm have a convention to use `##` for "real" comments, which I guess can make things slightly better, but I still haven't seen comments this big there...

So overall, I think this version is better than what you had before, but it still doesn't convince me that this is better than python.



================
Comment at: lldb/include/lldb/Host/DebugInfoD.h:16-18
+namespace llvm {
+class Error;
+} // End of namespace llvm
----------------
I guess this is not needed now.


================
Comment at: lldb/packages/Python/lldbsuite/test/httpserver.py:75
+    # Block only for 0.5 seconds max
+    httpd.timeout = 0.5
+    # Allow for reusing the address
----------------
What exactly is this timeout for? It seems rather small...


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:4
 
-The concrete subclass can override lldbtest.TesBase in order to inherit the
+The concrete subclass can override lldbtest.TestBase in order to inherit the
 common behavior for unitest.TestCase.setUp/tearDown implemented in this file.
----------------
just commit this separately.  no review needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750