kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/SourceCode.cpp:1241 + !HeaderInfo.hasFileBeenImported(FE) && + (FID != SM.getMainFileID() || !fileContainsImports(FID, SM))) return false; ---------------- can you put a comment here saying `any header that contains #imports are supposed to be #import'd so no need to check for anything but the main-file.` ================ Comment at: clang-tools-extra/clangd/SourceCode.cpp:1249 +bool fileContainsImports(FileID ID, const SourceManager &SM) { + llvm::StringRef Content = SM.getBufferData(ID); + llvm::StringRef Line; ---------------- nit: perform `.take_front` directly here. ================ Comment at: clang-tools-extra/clangd/index/Merge.cpp:251 SI.References += OI.References; + SI.SupportedDirectives = + SI.SupportedDirectives | OI.SupportedDirectives; ---------------- nit: `SI.SupportedDirectives |= OI.SupportedDirectives;` ================ Comment at: clang-tools-extra/clangd/index/Symbol.h:90 + enum IncludeDirectives : uint8_t { + Invalid = 0, ---------------- nit: I'd still keep the enum name singular ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:103 + using SK = index::SymbolKind; + switch (Kind) { + case SK::Macro: ---------------- can we rather modify the previous function to look like: ``` IncludeDirective shouldCollectIncludePath(index::SymbolKind Kind) { using SK = index::SymbolKind; switch (Kind) { case SK::Macro: case SK::Enum: case SK::Struct: case SK::Class: case SK::Union: case SK::TypeAlias: case SK::Using: case SK::Function: case SK::Variable: case SK::EnumConstant: case SK::Concept: return Include | Import; case SK::Protocol: return Import; default: return Invalid; } } ``` ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:838 + IncludeFiles[S.ID] = FID; + if (S.SymInfo.Lang == index::SymbolLanguage::ObjC) + FilesWithObjCConstructs.insert(FID); ---------------- can we do this in `addDeclaration` instead? we already have nameLocation and FileID there, but that's also where we have the decl itself, which might be needed in future if we need more detailed checks. ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:886 } + bool ContainsImports; + auto It = FileToContainsImports.find(Entry.second); ---------------- can we instead do ``` auto It = map.find(key); if (It == map.end()) { It = map.insert(key, calculateValue()).first; } bool ContainsImports = It->second; ``` ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:905 + if (shouldCollectImportPath(S->SymInfo.Kind) && + (ContainsImports || FilesWithObjCConstructs.contains(Entry.second))) + Directives |= Symbol::Import; ---------------- can we perform `ContainsImports` calculations/caching here instead? that way we can trim a bunch of redundant searches. ================ Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:431 } - return clangd::Symbol::IncludeHeaderWithReferences{Strings.save(Header), - Message.references()}; + auto IncludeTypes = fromProtobuf(Message.include_types()); + if (!IncludeTypes) ---------------- i guess `IncludeTypes` pieces are leftover? ================ Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:2679 + assert(!Results.empty()); + // TODO: Once #import integration support is done this shouldn't include + // anything. ---------------- i think this case should still be empty, otherwise we're actually regressing the behaviour by inserting includes for symbols that we previously wouldn't insert. 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