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
\p
+/// 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!
Done.
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42640
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
