[PATCH] D38639: [clangd] #include statements support for Open definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: unittests/clangd/XRefsTests.cpp:53
 
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+  void onDiagnosticsReady(
----------------
malaperle wrote:
> ilya-biryukov wrote:
> > NIT: remove this class, use `IgnoreDiagnostics` from `Compiler.h` instead.
> It looks like IgnoreDiagnostics from Compiler.h implements DiagnosticConsumer from Compiler.h, and not DiagnosticsConsumer from ClangdServer.h so that wouldn't work.
You're right, I got confused, sorry. Disregard my comment.


================
Comment at: unittests/clangd/XRefsTests.cpp:245
+  const char *SourceContents = R"cpp(
+  #include "$2^invalid.h"
+  #include "^foo.h"
----------------
malaperle wrote:
> ilya-biryukov wrote:
> > Could we also add tests for corner cases: cursor before opening quote, cursor after the closing quote, cursor in the middle of `#include` token? (we shouldn't navigate anywhere in the middle of the #include token)
> > cursor before opening quote, cursor after the closing quote
> 
> I assume we don't want to navigate anywhere for these positions? I don't have an opinion.
> 
> > (we shouldn't navigate anywhere in the middle of the #include token)
> 
> It did in CDT and I thought it worked nicely as it made it easier to click on it. You can hold ctrl on the whole line and it underlined it (well except trailing comments). But clients don't handle the spaces nicely (underline between #include and file name) so I thought I'd work on the client first before making the server do it. Anyhow, for now it shouldn't navigate indeed.
> > cursor before opening quote, cursor after the closing quote
>I assume we don't want to navigate anywhere for these positions? I don't have an opinion.
I'd say we should navigate when the cursor is right before the opening quote. For the closing quote I don't have any opinions either :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639