[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thank you for reviewing this!



================
Comment at: clangd/index/CanonicalIncludes.h:52
+  /// a canonical header name.
+  llvm::StringRef mapHeader(llvm::StringRef Header) const;
+
----------------
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > So I'm a bit concerned this is too narrow an interface, and we really want to deal with SourceLocation here which would give us the include stack.
> > > 
> > > Evidence #1: .inc handling really is the same job, but because this class has a single-file interface, we have to push it into the caller.
> > > Evidence #2: I was thinking about a more... principled way of determining system headers. One option would be to have a whitelist of public headers, and walk up the include stack to see if you hit one. (I'm not 100% sure this would work, but still...) This isn't implementable with the current interface.
> > > 
> > > One compromise would be to pass in a stack<StringRef> or something. Efficiency doesn't really matter because the caller can cache based on the top element.
> > > Evidence #1: .inc handling really is the same job, but because this class has a single-file interface, we have to push it into the caller.
> > I think this would depend on how you define the scope of this class. `.inc` handling is a subtle case that I'm a bit hesitated to build into the interface here.
> > 
> > > Evidence #2: ....
> > This is actually very similar to how the hardcoded mapping was generated. I had a tool that examined include stacks for a library (e.g. STL) and applied a similar heuristic - treat the last header in the include stack within the library boundary as the "exporting" public header for a leaf include header, if there is no other public header that has shorter distance to that include. For example, if we see a stack like `stl/bits/internal.h -> stl/bits/another.h -> stl/public_1.h -> user_code.cc`, we say `public_1.h` exports `bits/internal.h` and add a mapping from `bits/internal.h$` to `public_1.h`. But if we see another (shorter) include stack like `stl/bits/internal.h -> stl/public_2.h -> user_code.cc`, we say `stl/public_2.h` exports `stl/bits/internal.h`. This heuristic works well for many cases. However, this may produce wrong mappings when an internal header is used by multiple public headers. For example, if we have two include stacks with the same length e.g. `bits/internal.h -> public_1.h -> user.cc` and `bits/inernal.h -> public_2.h -> user.cc`, the result mapping would depend on the order in which we see these two stacks; thus, we had to do some manual adjustment to make sure bits/internal.h is mapped to the correct header according to the standard.
> > 
> > I am happy to discuss better solution here. But within the scope of this patch, I'd prefer to stick to interfaces that work well for the current working solution instead of designing for potential future solutions. I should be easy to iterate on the interfaces as these interfaces aren't going to be widely used in clangd after all. WDYT?
> > I think this would depend on how you define the scope of this class. .inc handling is a subtle case that I'm a bit hesitated to build into the interface here.
> 
> Sure it's subtle, but it's clearly in the scope of determining what the canonical header is for a symbol, which is the job of this class. We wouldn't be building it into the interface - on the contrary, the *current* proposed interface codifies *not* handling .inc files.
> 
> But you're right that we should check in something that handles most cases.
> 
> My preference would be to drop `.inc` from this patch until we can incorporate it into the design, but I'm also OK with a FIXME to move it.
Okay, I removed `.inc` handling from this patch ;) 


================
Comment at: unittests/clangd/HeadersTests.cpp:29
+  // absolute path.
+  std::string addToSubdir(PathRef File, llvm::StringRef Code = "") {
+    assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
----------------
sammccall wrote:
> sammccall wrote:
> > This test would be clearer to me if you removed this helper and just did
> > 
> > ```FS.Files["sub/bar.h"] = ...```
> > 
> > in the test.
> > 
> > Can we change `buildTestFS` in `TestFS.cpp` to call `getVirtualTestFilePath` on relative paths to allow this?
> > 
> > (I can do this as a followup if you like, but it seems like a trivial change)
> I thought better of that change to TestFS, but did some renames in r325326.
> 
> So this would be `FS.Files[testPath("sub/bar.h")) = ...` which still seems more transparent - up to you.
Thanks a lot!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640