This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Skip .inc headers when canonicalizing header #include.
ClosedPublic

Authored by ioeric on May 22 2018, 1:41 AM.

Event Timeline

ioeric created this revision.May 22 2018, 1:41 AM
sammccall accepted this revision.May 23 2018, 2:50 AM

n

clangd/index/CanonicalIncludes.cpp
50

nit: here and elsewhere, you probably want "declaring" rather than defining

clangd/index/CanonicalIncludes.h
52

This might be clearer slightly more concretely:

\p Headers is the include stack: Headers.front() is the file declaring the symbol, and Headers.back() is the main file

(or "included by the main file", but having it be main-file might be neater)

54

Nit: "this" is a bit ambiguous here, the parameter or the algorithm?
Consider just dropping this new part of the comment, it's kind of an implementation detail (hiding behind the word "canonical")

clangd/index/SymbolCollector.cpp
209

(as above, maybe want to include the main file for simplicity/symmetry)

This revision is now accepted and ready to land.May 23 2018, 2:50 AM
ioeric updated this revision to Diff 148189.May 23 2018, 5:05 AM
ioeric marked 4 inline comments as done.
  • Include main file in the include stack when mapping headers.
clangd/index/SymbolCollector.cpp
209

Thanks! As you pointed out offline, we would need main file for correctness as well, as a main file could the exporting header.

sammccall added inline comments.May 23 2018, 7:50 AM
clangd/index/CanonicalIncludes.cpp
52

headers are paths, not <quoted>, right?

53

This logic could be merged with the .inc logic, right?
"Find the first header such that the extension is not '.inc', and isn't a recognized non-header file"

58

lookupTypeForExtension can return TY_INVALID, which you have to check for (onlyPrecompileType will assert).

I think we should be conservative and accept TY_INVALID as possible headers - our goal here really is to exclude "obvious" non-headers like cpp.

(in this case you can probably drop the explicit empty check)

ioeric updated this revision to Diff 148218.May 23 2018, 8:13 AM
ioeric marked 3 inline comments as done.
  • addressed comments.
This revision was automatically updated to reflect the committed changes.

I think this might have broken the "shared lib" build? Could you double check that you don't need to add "clangDriver" ?

../tools/clang/tools/extra/clangd/index/CanonicalIncludes.cpp:61: error: undefined reference to 'clang::driver::types::lookupTypeForExtension(llvm::StringRef)'
../tools/clang/tools/extra/clangd/index/CanonicalIncludes.cpp:63: error: undefined reference to 'clang::driver::types::onlyPrecompileType(clang::driver::types::ID)'

Thanks!


From: cfe-commits <cfe-commits-bounces@lists.llvm.org> on behalf of Eric Liu via Phabricator via cfe-commits <cfe-commits@lists.llvm.org>
Sent: Thursday, May 24, 2018 10:44:26 AM
To: ioeric@google.com; sammccall@google.com
Cc: maskray@google.com; llvm-commits@lists.llvm.org; cfe-commits@lists.llvm.org
Subject: [PATCH] D47187: [clangd] Skip .inc headers when canonicalizing header #include.

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333188: [clangd] Skip .inc headers when canonicalizing header #include. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:

rL LLVM

https://reviews.llvm.org/D47187

Files:

clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp