kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
================
Comment at: clang-tools-extra/clangd/test/lit.cfg.py:26
config.clangd_binary_dir + "/benchmarks"))
+config.substitutions.append(('%python',
+ config.python_executable))
----------------
we should rather call `lit.llvm.llvm_config.use_default_substitutions()`,
preferably right after `use_clang()` on line 4.
sorry for the churn, i thought we were already doing this.
================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:19
+def main():
+ server_address = 'localhost:'
+ # Grab an available port.
----------------
nit: maybe perform the magic after arg parsing ?
================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:22
+ with socket() as s:
+ s.bind(('', 0))
+ server_address += str(s.getsockname()[1])
----------------
this still binds the socket to all interfaces, can we specify `localhost` or
`127.0.0.1` explicitly for the interface address.
================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:36
+ ],
+ stderr=subprocess.PIPE)
+
----------------
nit: formatting looks off, is this really what yapf offers?
================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:39
+ # Wait for the server to warm-up.
+ if not b'Server listening' in index_server_process.stderr.readline():
+ sys.exit(1)
----------------
i would rather scan through all lines and look for `Server listening on
$server_addres`. We should be able to either hit the end of stream (i.e. server
encounters a failure and shuts down) or find the log message we are looking for.
just to make test less reliant on log ordering.
================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:51
+ clangd_process.wait()
+ os.kill(index_server_process.pid, 9)
+
----------------
can we rather use `signal.SIGXXX` here instead of `9` ?
Also rather than kill, SIGINT might be more applicable.
https://docs.python.org/3/library/signal.html#signal.SIGKILL claims sigkill is
not available on windows.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90291/new/
https://reviews.llvm.org/D90291
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
