sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land.
In https://reviews.llvm.org/D52547#1247794, @kadircet wrote: > In https://reviews.llvm.org/D52547#1246701, @ilya-biryukov wrote: > > > A drive-by comment. > > Would it be cleaner to pass this information from clang? Relying on > > completion label seems shaky. > > > Actually I also wanted to do that at first, but then wasn't really sure > whether we should introduce a new contextkind or not, because it is more > about lsp and wasn't sure if it would be widely applicable to have a > distinction between file/directory(folder) as a completion context kind. > WDYT @sammccall , would introducing a new completion context carry its > weight? What Ilya proposes would indeed be cleaner - I think we could attach directory_entry or path/file_type to the completion result. On the other hand while this is a hack, it's simple and it works, and the cleaner solution doesn't bring any concrete benefit at this point. I think we should leave this be and revisit if we start querying headers from the index. (At that point we'll want more robust path info to deduplicate sema and index results). ================ Comment at: clangd/CodeComplete.cpp:352 C.SemaResult->Kind, C.SemaResult->Declaration, ContextKind); + if (Completion.Kind == CompletionItemKind::File && + Completion.Name.back() == '/') ---------------- this probably deserves at least a comment mentioning that Sema could provide more semantic info here. ================ Comment at: clangd/Protocol.h:706-707 File = 17, Reference = 18, + Folder = 19, }; ---------------- This is a type outside of the core must-be-supported range, so we probably need to add the logic to ClangdLSPServer to respect the client capabilities (see `adjustKindToCapability`). This could be a separate patch, as it's basically plumbing. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52547 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
