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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
