sammccall marked 5 inline comments as done.
sammccall added a comment.
Oops, took a while to get back to this.
Planning to land this with some style unaddressed (%/S, --input-file) but
please do LMK if they're important and I'll fix.
================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:31
+ parser.add_argument('--server-arg', action='append')
+ parser.add_argument('--server-log', nargs='?', type=argparse.FileType('wb'),
default=os.devnull)
----------------
kbobyrev wrote:
> Why do we need `wb` here instead of `w`? I know that `subprocess.Popen`
> writes binary data but since we're writing actual text to file maybe we could
> convert to non-binary?
we're just redirecting data from one stream to another, I don't think there's
any reason to do character decoding+encoding on the way...
================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:81
if __name__ == '__main__':
main()
----------------
kbobyrev wrote:
> this was `pipeline_helper.py` intended for `pipeline.test`, maybe we'll use
> it more so makes sense to rename it to just `helper.py` or somethting.
Agree - can do that as a followup.
================
Comment at: clang-tools-extra/clangd/test/remote-index/public-log.test:2
+# RUN: rm -rf %t
+# RUN: clangd-indexer %S/Inputs/Source.cpp > %t.idx
+# RUN: %python %S/pipeline_helper.py --input-file-name=%s
--server-arg=--log=verbose --server-arg=-log-public --server-log=%t.public.log
--project-root=%S --index-file=%t.idx > /dev/null
----------------
kadircet wrote:
> nit: use `%/S` to not mix forward and backslashes.
Why do we want to avoid mixing them?
(This is copied from the other test which uses %S)
================
Comment at: clang-tools-extra/clangd/test/remote-index/public-log.test:3
+# RUN: clangd-indexer %S/Inputs/Source.cpp > %t.idx
+# RUN: %python %S/pipeline_helper.py --input-file-name=%s
--server-arg=--log=verbose --server-arg=-log-public --server-log=%t.public.log
--project-root=%S --index-file=%t.idx > /dev/null
+# RUN: %python %S/pipeline_helper.py --input-file-name=%s
--server-arg=--log=verbose --server-log=%t.log --project-root=%S
--index-file=%t.idx > /dev/null
----------------
kadircet wrote:
> not sure if it is any easier, but it might make sense to just pass anything
> after `--` to server, rather than explicitly mentioning `--server-arg` before
> each one. I suppose we can also rename the script to `server_request_helper` ?
>
> This won't work if we decide to pass arbitrary client flags too though.
Yeah, there aren't a lot of these so i'd like it to be super-explicit for now.
Happy to change this if we end up with lots of tests passing various server
args (and few client args)
================
Comment at: clang-tools-extra/clangd/test/remote-index/public-log.test:5
+# RUN: %python %S/pipeline_helper.py --input-file-name=%s
--server-arg=--log=verbose --server-log=%t.log --project-root=%S
--index-file=%t.idx > /dev/null
+# RUN: FileCheck --check-prefixes=LOG,LOG-PUBLIC %s < %t.public.log
+# RUN: FileCheck --check-prefixes=LOG,LOG-ALL %s < %t.log
----------------
ArcsinX wrote:
> Maybe we can use `--input-file` option of FileCheck instead of `<` (the same
> for the next line)
Why would this be preferred? (`<` is roughly 5x more common in llvm/test and
clang/test - neither is used significantly in clang-tools-extra)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90654/new/
https://reviews.llvm.org/D90654
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
