[PATCH] D90654: [clangd] Add index server request logging

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev accepted this revision.
kbobyrev added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:257
+
+  public:
+    TextProto(const google::protobuf::Message &M) : M(M) {}
----------------
nit: everything is public by default since it's `struct`, right? did you mean to put `&M` behind `private`?


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:25
 
-
 def main():
----------------
nit: PEP is against having <2 lines between functions :(


================
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)
 
----------------
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?


================
Comment at: clang-tools-extra/clangd/test/remote-index/pipeline_helper.py:81
 if __name__ == '__main__':
   main()
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90654