kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:237
for (const auto &Include : Includes)
- Headers.push_back(Include.IncludeHeader);
+ if (Include.SupportedDirectives & clang::clangd::Symbol::Include)
+ Headers.push_back(Include.IncludeHeader);
----------------
i don't think this is the right layer to perform such a filtering, we should
instead be returning everything here, both the header and the directive to use.
later on inside CodeComplete.cpp, there's `headerToInsertIfAllowed` for now we
can drop headers to be #import'd there, with a fixme saying propagation into
other layers.
similarly IncludeFixer should drop these inside `fixesForSymbols` for now.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:809
void SymbolCollector::setIncludeLocation(const Symbol &S, SourceLocation Loc) {
if (Opts.CollectIncludePath)
+ if (shouldCollectIncludePath(S.SymInfo.Kind) != Symbol::Invalid)
----------------
nit: while here combine with the condition above
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:843
llvm::SmallString<128> QName;
for (const auto &Entry : IncludeFiles) {
if (const Symbol *S = Symbols.find(Entry.first)) {
----------------
nit: feel free to rewrite as `const auto &[SID, FID] : IncludeFiles`, as you
seem to be referring to `Entry.second` a bunch.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:870
+ if ((CollectDirectives & Symbol::Import) != 0) {
+ auto It = FileToContainsImportsOrObjC.find(Entry.second);
+ if (It == FileToContainsImportsOrObjC.end())
----------------
nit:
```
if ((CollectDirectives & Symbol::Import) != 0) {
auto [It, Inserted] =
FileToContainsImportsOrObjC.try_emplace(Entry.second);
if(Inserted)
It->second = FilesWithObjCConstructs.contains(Entry.second) ||
fileContainsImports(Entry.second,ASTCtx->getSourceManager());
if(It->second) Directives |= Symbol::Import;
}
```
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:877
+ ASTCtx->getSourceManager()) ||
+ FilesWithObjCConstructs.contains(Entry.second)})
+ .first;
----------------
i'd first do this check, as it doesn't require parsing file contents
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:966
setIncludeLocation(S, ND.getLocation());
+ if (Opts.CollectIncludePath && S.SymInfo.Lang == index::SymbolLanguage::ObjC)
+ FilesWithObjCConstructs.insert(FID);
----------------
let's drop the `Opts.CollectIncludePath` check. it doesn't really align with
the model of "we're just marking files that contain objc constructs"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128457/new/
https://reviews.llvm.org/D128457
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
