[PATCH] D128457: [clangd] Add new IncludeType to IncludeHeaderWithReferences

[Kadir Cetinkaya via Phabricator via cfe-commits]

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