VitaNuo added a comment.
Thanks for comments!
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:40
/// Adds a file-to-string mapping from \p ID to \p CanonicalPath.
void addMapping(FileEntryRef Header, llvm::StringRef CanonicalPath);
----------------
kadircet wrote:
> AFAICT, only users of this endpoint were in `IWYUCommentHandler` (and some
> tests). we can also get rid of this one now.
>
> More importantly now this turns into a mapper used only by symbolcollector,
> that can be cheaply created at use time (we just copy around a pointer every
> time we want to create it). so you can drop `CanonicalIncludes` from all of
> the interfaces, including `SymbolCollector::Options` and create one on demand
> in `HeaderFileURICache`. all we need is langopts, and it's available through
> preprocessor (not necessarily on construction, but at the time when we want
> to do the mappings).
>
> As you're already touching all of the interfaces that propagate
> `CanonicalIncludes` around, hopefully this change should only make things
> simpler (and you already need to touch them when you rebase), but if that
> feels like too much churn in this patch feel free to do that in a follow up
> as well.
> ... and create one on demand in HeaderFileURICache. all we need is langopts,
> and it's available through preprocessor (not necessarily on construction, but
> at the time when we want to do the mappings
This time sounds a little late, since we don't want to rebuild the system
header mapping every time we request a mapping for a particular header.
Cache construction time doesn't work, since it happens before the preprocessor
is set in the symbol collector (and we need the preprocessor to retrieve
language options).
So far the safe place I've found is right after the preprocessor is set in the
symbol collector. Another option is to collect the system header options in the
finish() method, but I can't see why we would need to delay until then.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:250
+ // of whether it's an otherwise-good header (header guards etc).
+ llvm::StringRef mapCanonical(FileID FID) {
+ if (SysHeaderMapping) {
----------------
kadircet wrote:
> let's change the interface here to take in a `FileEntry` instead.
I believe it has to be `FileEntryRef` instead. This is what the
`CanonicalIncludes::mapHeader` expects, and this is also seems right (somewhere
in the Clang docs I've read "everything that needs a name should use
`FileEntryRef`", and name is exactly what `CanonicalIncludes::mapHeader`
needs).
For the case of a physical header (i.e., `FileEntry` instance) I've switched to
using `getLastRef` now. There's also another API `FileManager::getFileRef`
where I could try passing the result of `tryGetRealPathName`. I am not sure I
have enough information to judge if any of these options is distinctly better.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:412-413
+
+ if (auto Verbatim = PI->getPublic(FileEntry); !Verbatim.empty())
+ return Verbatim;
+
----------------
kadircet wrote:
> let's move this logic into `mapCanonical` too.
This would deliver wrong results for C++. We would basically get every
IWYU-public header twice: once through the `verbatim` branch of the
include_cleaner results, and then once again when mapping the physical private
file to a canonical version (e.g., for system headers).
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:849
+ shouldCollectIncludePath(S.SymInfo.Kind) != Symbol::Invalid)
+ SymbolProviders[S.ID] = std::move(Headers);
+}
----------------
kadircet wrote:
> because you're taking in `Headers` as `const`, this move is not actually
> moving.
Ok, this is not relevant anymore, but I am not sure I understand why. Am I not
allowed to say that I don't need this variable/memory anymore, even though it's
`const`?
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:877
// We delay this until end of TU so header guards are all resolved.
for (const auto &[SID, FID] : IncludeFiles) {
if (const Symbol *S = Symbols.find(SID)) {
----------------
kadircet wrote:
> let's iterate over `SymbolProviders` instead, as `IncludeFiles` is a legacy
> struct now.
Not sure I'm following. Iterating over `SymbolProviders` means retrieving
`include_cleaner::Header`s. How would I handle the entire Obj-C part then,
without the FID of the include header?
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:930
+ auto Canonical =
+ HeaderFileURIs->mapCanonical(SM.getOrCreateFileID(
+ H.physical(), SrcMgr::CharacteristicKind::C_User));
----------------
kadircet wrote:
> no need for the `getOrCreateFileID` after you change parameter for
> `mapCanonical` to be a file entry.
Still need the `getLastRef` or `FileManager::getFileRef`, if I understand
correctly.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:175
SymbolSlab::Builder Symbols;
- // File IDs for Symbol.IncludeHeaders.
- // The final spelling is calculated in finish().
+ // File IDs to distinguish imports from includes.
llvm::DenseMap<SymbolID, FileID> IncludeFiles;
----------------
kadircet wrote:
> we seem to be populating this map for all kinds of symbols, as long as
> `shouldCollectIncludePath` returns non-invalid. what's up with distinguishing
> `imports from includes`?
Yeah that's right. I've been trying to express that (after populating this map)
we are not using this `FID` to compute the IncludeHeaders anymore (only for the
ObjC case), but rather use it to determine if the `Directives` should contain
`Symbol::Import` (ie., if this FID contains Obj-C constructs/imports).
I'll try to be a bit more verbose and precise.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152900/new/
https://reviews.llvm.org/D152900
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
