VitaNuo added a comment.
Thanks for the comments!
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:393-395
+ const auto *FileEntry = SM.getFileEntryForID(FID);
+ for (const auto *Export : PI.getExporters(FileEntry, SM.getFileManager()))
+ return toURI(Export->tryGetRealPathName());
----------------
kadircet wrote:
> sorry i don't understand what this logic is trying to achieve.
>
> we seem to be prefering "first" exporting header, over the headers that
> directly provide the symbol. Also comment says it's done for objc, but
> there's nothing limiting this logic to ObjC. any chance this is unintended?
No, this was intended, but possibly I didn't get it right.
> comment says it's done for objc, but there's nothing limiting this logic to
> objc
AFAIU this whole code path will now only be reachable for objc symbols. I have
removed pragma includes from the canonical include mapping now, leaving the
pragma handling to include cleaner. However, that only triggers for C/C++, so
in case we need the `export` and `private` pragmas for objc, we need to
re-insert the handling for them somewhere.
There is no pre-existing test case for pragmas in objc and there is no usage of
these pragmas for objc code in the code base either, so I have no way of
knowing if this is actually needed. But I believe you've mentioned in some
discussion we should still handle the pragmas for objc.
> we seem to be preferring "first" exporting header, over the headers that
> directly provide the symbol.
Isn't that correct if there's an `IWYU export` pragma involved? The snippet
comes from `include_cleaner::findHeaders`, with the only difference that it
stops at the first exporter (since the canonical include mapping also just
stored one mapping for the header).
Let me know how to do it better, or maybe if this is necessary at all.
Honestly, I am not sure about this, since, as mentioned, there are no `export`
or `private/public` pragmas in objc files in the codebase atm.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:903
+ NewSym = *S;
+ if (!IncludeHeader.empty()) {
NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives});
----------------
kadircet wrote:
> this is using legacy mappings for non-objc symbols too
Removed this whole passage.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:934-936
+ SymbolIncludeSpelling[SID] = HeaderFileURIs->getIncludeHeader(
+ ASTCtx->getSourceManager().getOrCreateFileID(
+ H.physical(), SrcMgr::CharacteristicKind::C_User));
----------------
kadircet wrote:
> we actually only want to use `HeaderFileURIs->toURI` here and not the
> `getIncludeHeader`, because:
> - We want to make use of mapping logic in include-cleaner solely, and we
> already have all that information in Providers mapping.
> - The extra logic we want to apply for ObjC symbols are already applied
> before reaching here.
>
> the way we create a FileID here is a little bit iffy, by using `toURI`,
> hopefully we can avoid that conversion.
>
> the only remaining issue is, we actually still want to use system header
> mappings inside `CanonicalIncludes` until we have better support for them in
> include-cleaner library itself. So I think we should still call
> `Includes->mapHeader(H.physical())` before using `toURI`.
Ok SGTM. It seems, though, that we still need the "iffy" FID for the canonical
mapping.. Let me know if you know a better way.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:131
+ }
+ void setPreprocessor(Preprocessor &PP) {
+ this->PP = &PP;
----------------
kadircet wrote:
> unfortunately this alternative is called after we've parsed the file, from
> `clangd/index/FileIndex.cpp`, `indexSymbols`. hence attaching `PI` to
> `PPCallbacks` won't have any effect.
>
> This is working unintentionally because we're still populating
> `CanonicalIncludes` with legacy IWYUPragmaHandler in clangd, and using that
> to perform private -> public mappings.
>
> We should instead propagate the `PragmaIncludes` we have in `ParsedAST` and
> in preamble builds here. You should be able to test the new behaviour
> `clang-tools-extra/clangd/unittests/FileIndexTests.cpp` to make sure pragma
> handling through dynamic indexing code paths keep working as expected using
> an `export` pragma.
Ok thank you. I'm using `PragmaIncludes` from AST and preamble builds for the
dynamic index now.
For the background index, I've finally managed to move pragma recording to the
`IndexAction` now.
I've also removed the redundant (to include cleaner) comment handlers from the
AST build and preamble building logic.
Also removed the `mapSymbol` method from `CanonicalIncludes`, since it had one
usage which should now be covered by the include cleaner, I believe.
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
