[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/CodeComplete.cpp:755
+      });
+      log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+          "symbols with non-empty docs in the response",
----------------
ioeric wrote:
> hokein wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > drive by: I think this should  be `vlog` or `dlog`.
> > > > Code completion also logs the number of results from sema, index, etc. using the `log()` call.
> > > > The added log message looks similar, so trying to be consistent with the rest of the code in this file.
> > > > 
> > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave it to a separate patch.
> > > I'm not sure which level code completion log messages should be in, but I think we should follow the guidelines in the logger documentation instead of the existing uses.
> > > 
> > > Quote from Logger.h
> > > ```
> > > // log() is used for information important to understanding a clangd session.
> > > // e.g. the names of LSP messages sent are logged at this level.
> > > // This level could be enabled in production builds to allow later inspection.
> > > 
> > > // vlog() is used for details often needed for debugging clangd sessions.
> > > // This level would typically be enabled for clangd developers.
> > > ```
> > My recent experience of debugging some weird GotoDef issues tells me that log of index should be on production (since it is a non-trivial part in a clangd session), it would be really helpful to understand what is going on. 
> I agree that they are useful for debugging, but they might not be interesting to end-users. And I think that's why there is `vlog`. Clangd developers could use a different log level with `--log` flag.
I agree that `vlog` is probably a better fit here, but still think we should change it across `CodeComplete.cpp` in a single commit, rather than partially apply the guidelines to different parts of the file.

However, if everyone agrees we should use `vlog` here, happy to use `vlog` too and also send a patch to update all the rest of the usages.
The situation I'm trying to avoid is:
1. We commit `vlog` here.
2. Someone argues that using `log` is actually better and we decide to not change other usages to `log`.
3. We end up with inconsistent choices across this file: `vlog` for signature help and `log` for code completion.

But if there's an agreement that we should use `vlog` everywhere, more than happy to change it :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50727