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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
