sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG apart from the .inc handling (happy to chat more)
================
Comment at: clangd/index/CanonicalIncludes.h:52
+ /// a canonical header name.
+ llvm::StringRef mapHeader(llvm::StringRef Header) const;
+
----------------
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.
================
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");
----------------
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)
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:50
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+ return arg.Detail && !arg.Detail->IncludeHeader.empty();
----------------
nit: this is only used once, as Not(HasIncludeHeader()). Just use
IncludeHeader("")?
The slight difference in detail handling doesn't seem to matter (I'm not even
sure if it's exactly the right assertion)
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:624
+ TestHeaderURI = URI::createFile(TestHeaderName).toString();
+ runSymbolCollector("class Foo {};", /*Main=*/"");
+ EXPECT_THAT(Symbols,
----------------
Took me a while to understand this test, and still not sure I get it. Maybe
"class string" here?
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
