sammccall added a comment.
Some comments on the insert side, which looks pretty good. I'll take another
look at indexing tomorrow.
================
Comment at: clangd/ClangdServer.cpp:465
+ auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo();
+ std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+ *Resolved, CompileCommand.Directory);
----------------
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.
================
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
----------------
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!
================
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
----------------
maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation) and add
an example?
It might be worth explicitly stating that the result is an #include string
(with quotes) - you just call it a "path" here.
"shortest" makes sense as a first implementation, but we may want to document
something more like "best" - I know there are codebases we care about where
file-relative paths are banned. Also I suspect we don't give
"../../../../Foo.h" even if it's shortest :-)
================
Comment at: clangd/ClangdServer.cpp:379
+ llvm::StringRef Header) {
+ // In order to get a HeaderSearch instance, we first create a
CompilerInstance
+ // and initialize it to get a Preprocessor which provides HeaderSearch.
----------------
This comment helps a lot. The subtext is: HeaderSearch is hard to construct
directly, so we're doing this weird dance.
I think this is worth calling out even louder - when I read this sort of code I
tend to take a *long* time to work out why the code seems to be doing unrelated
work.
================
Comment at: clangd/ClangdServer.cpp:420
+ auto &HeaderSearchInfo = Clang->getPreprocessor().getHeaderSearchInfo();
+ std::string Suggested = HeaderSearchInfo.suggestPathToFileForDiagnostics(
+ Header, CompileCommand.Directory);
----------------
consume the optional `IsSystem` and use it to quote appropriately?
================
Comment at: unittests/clangd/ClangdTests.cpp:849
+TEST_F(ClangdVFSTest, InsertIncludes) {
+ MockFSProvider FS;
----------------
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)
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
