The IncludeType contains both Include (the current behavior) and Import,
which we can use in the future to provide #import suggestions for
Objective-C files/symbols.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clangd/index/Symbol.h | ||
---|---|---|
96 | you can use LLVM_MARK_AS_BITMASK_ENUM and relevant magic to implement bitwise operators on the type (see llvm-project/llvm/include/llvm/ADT/BitmaskEnum.h) | |
119–126 | i think we already rely on unsigned being 32 bits here. instead of introducing a new byte, can we make References 30 bits long and use 2 bits for the include type? also change the type from unsigned to uint32_t. | |
122 | the naming here feels a little confusing, can we change the enum name to be IncludeDirective and field name to SupportedDirectives | |
clang-tools-extra/clangd/index/remote/Index.proto | ||
110 | no need for the comment here. | |
clang-tools-extra/clangd/unittests/SerializationTests.cpp | ||
59 | can we also have an example for both (and none) |
Fixes for review
- Swap over IncludeType to IncludeDirective and update the protos accordingly
Also LMK if you want more in this change (such as a flag to control it, just not sure where it should live + what it should be called).
clang-tools-extra/clangd/index/Symbol.h | ||
---|---|---|
122 | Done, although for Serialization I left it as a 32 bit and then 8 bit include directives, LMK if I should swap it over to serialize as a 32 bit single value... Guess I would need to use a union or manually bit manipulate it to store + load it? | |
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp | ||
1689–1690 ↗ | (On Diff #473347) | Not sure the best way to fix this, should we also scan the -include files for #imports? |
Fix serialization and isSelfContainedHeader
- Keep serialization as a single var uint32
- Keep old imported logic in isSelfContainedHeader in addition to a new check for import lines only for main files
clang-tools-extra/clangd/SourceCode.cpp | ||
---|---|---|
1241 ↗ | (On Diff #475180) | 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. |
1249 ↗ | (On Diff #475180) | nit: perform .take_front directly here. |
clang-tools-extra/clangd/index/Merge.cpp | ||
251 | nit: SI.SupportedDirectives |= OI.SupportedDirectives; | |
clang-tools-extra/clangd/index/Symbol.h | ||
90 | nit: I'd still keep the enum name singular | |
clang-tools-extra/clangd/index/SymbolCollector.cpp | ||
106 | 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; } } | |
812 | 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. | |
860 | can we instead do auto It = map.find(key); if (It == map.end()) { It = map.insert(key, calculateValue()).first; } bool ContainsImports = It->second; | |
870 | can we perform ContainsImports calculations/caching here instead? that way we can trim a bunch of redundant searches. | |
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp | ||
431–436 | i guess IncludeTypes pieces are leftover? | |
clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp | ||
2736 | 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. |
clang-tools-extra/clangd/Headers.cpp | ||
---|---|---|
225 | 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. | |
clang-tools-extra/clangd/index/SymbolCollector.cpp | ||
809–810 | nit: while here combine with the condition above | |
843–844 | nit: feel free to rewrite as const auto &[SID, FID] : IncludeFiles, as you seem to be referring to Entry.second a bunch. | |
871 | 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; } | |
878 | i'd first do this check, as it doesn't require parsing file contents | |
964 | 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" |
it feels like rebasing went wrong. changes from 2 unrelated patches seem to be part of this one now. can you make sure this patch only contains the delta for symbolcollector/clangd pieces?
clang-tools-extra/clangd/Headers.h | ||
---|---|---|
48 | let's rename this to SymbolInclude as it looks too similar to HeaderFile right now. also adding a comment like: A header and directives as stored in a Symbol. | |
50 | let's mention that this is either a URI or verbatim include in the comments | |
clang-tools-extra/clangd/index/SymbolCollector.cpp | ||
870 | can we add a comment like Only allow #import for symbols from objc-like files. | |
clang/lib/Tooling/Inclusions/HeaderAnalysis.cpp | ||
76 | SM.getFileEntryForID(SM.getMainFileID()) == FE |
let's rename this to SymbolInclude as it looks too similar to HeaderFile right now.
also adding a comment like: A header and directives as stored in a Symbol.