ioeric added a comment.

Thanks! PTAL

Comment at: clangd/ClangdServer.cpp:465
+    auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo();
+    std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+        *Resolved, CompileCommand.Directory);
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > do we handle the case that the suggestion is already included?
> > > (including the case where it's included by a different name)
> > Yes and no. The current implementation only does textual matching against 
> > existing #includes in the current file and inserts the header if no same 
> > header was found. This complies with IWYU. But we are not handling the case 
> > where the same header is included by different names. I added a `FIXME` for 
> > this.
> I can't see the FIXME? (There's one in the header, but it doesn't seem to 
> really cover this case)
> So this doesn't seem so hard: we can pass the file content, turn off 
> recursive PP, and add PP callbacks to capture the names of each included file 
> (the include-scanner patch does this).
> I'm not sure it's worth deferring, at least we should fix it soon before we 
> lose context.
> But up to you, I'd suggest putting the fixme where we expect to fix it.
Since you prefer, I have included the change in the patch. I wanted to get to 
this as soon as this patch is landed. 

Comment at: clangd/ClangdServer.cpp:368
+/// Calculates the shortest possible include path when inserting \p Header to 
+/// File, by matching \p Header against all include search directories for \p
sammccall wrote:
> This has a fair amount of logic (well, plumbing :-D) and uses relatively 
> little from ClangdServer.
> Can we move `shortenIncludePath` to a separate file and pass in the FS and 
> command?
> I'd suggest maybe `Headers.h` - The formatting part of `insertInclude` could 
> also fit there, as well as probably the logic from `switchSourceHeader` I 
> think (the latter not in this patch of course).
> This won't really help much with testing, I just find it gets hard to 
> navigate files that have lots of details of unrelated features. ClangdUnit 
> and ClangdServer both have this potential to grow without bound, though 
> they're in reasonable shape now. Interested what you think!

I didn't move the formatting code, as half of the code is pulling out the 
style, which we might want to share/change depending on other clangd logics 
that might use style.

Comment at: unittests/clangd/ClangdTests.cpp:849
+TEST_F(ClangdVFSTest, InsertIncludes) {
+  MockFSProvider FS;
sammccall wrote:
> Similarly, it'd be nice to pull these tests out into a test file parallel to 
> the header.
> (As with the other tests, often it's easiest to actually test through the 
> ClangdServer interface - this is mostly just for navigation)
Done. I left a simple test for replacements.

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to