[PATCH] D93763: [clangd] Add a flag to disable the documentLinks LSP request

[Giulio Girardi via Phabricator via cfe-commits]

rapgenic marked 4 inline comments as done.
rapgenic added a comment.

> In the future it may make sense to have other documentLinks too. (e.g. imagine a comment Configures the Frobnicator - we should linkify Frobnicator to point to a class, once documentLink supports target ranges). So we should decide if this feature is "disable documentLink" or "don't include included headers in documentLink". This affects e.g. naming of the flag, which is awkward to change later.

I think it's already like that, in that specific case, and it uses goToDefinition instead: if I ctrl+click on a variable/class/function name in a comment i can actually go to the definition, if I am not wrong.

As for the naming of the option, maybe the title of the patch is not too accurate, however from the option description it's quite clear that it's only related to include files, so any additional feature using documentLinks should be easy to add while keeping this option too.

> Do we really want to add command-line flags for this or does it rather belong as a config option? (In which case we'd want to change the behavior of the method, not turn the method itself on/off, to allow the config option to vary across files in the usual way).

In my opinion (and also by looking at the already implemented options in .clangd config file) the config file seems to be related to project specific configurations (compilation flags, indexing, etc.), whereas the option that this patch implements is more related to the "editing experience", which is something one should not change very often. So I think that it might not be worth putting it in the config file.



================
Comment at: clang-tools-extra/clangd/test/xrefs.test:4
 ---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"extern int x;\nint x = 0;\nint y = x;"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include <stdint.h>\nextern int x;\nint x = 0;\nint y = x;"}}}
 ---
----------------
sammccall wrote:
> Having tests depend on external headers is a bit of a pain downstream - it means they need to run in an environment where the headers are available.
> 
> We do this by necessity in `document-link.test` - maybe we should move this test here?
> (Personally I'm comfortable with this functionality only being covered by unit-tests, as it is now though).
I'm moving the test, as it is related to document links it seems pertinent to put it there, if you prefer we can leave it out of course


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

https://reviews.llvm.org/D93763