ilya-biryukov added inline comments.
================ Comment at: clangd/CodeComplete.cpp:334 + Item.insertTextFormat = InsertTextFormat::PlainText; + // FIXME: sort symbols appropriately. + Item.sortText = ""; ---------------- ioeric wrote: > ilya-biryukov wrote: > > NIT: FIXME(ioeric)? > > Also, why not provide some `sortText` here? Like a qualified name. Because > > we're currently missing scores? > Does `sortText` have to be non-empty? I thought the empty string would make > all candidates equally scored? > > I think we want either sensible score or (for now) no score at all. LS says that if `sortText` is empty, `label` should be used. We're probably fine here, you're right. ================ Comment at: clangd/CodeComplete.h:64 + + // Populated internally by clangd, do not set. + /// If `Index` is set, it is used to augment the code completion ---------------- ioeric wrote: > ilya-biryukov wrote: > > Given the comment, maybe we want to pass it as a separate parameter instead? > @sammccall suggested this approach in the previous prototype patch. I also > ended up preferring this approach because: > 1) it doesn't require changing ClangdServer interfaces, and the dynamic > index should be built by ClangdServer and thus doesn't make sense to be a > parameter. > 2) it enables unit tests to use customized dummy indexes. > 3) we might take another (static) index in the future, and it seems easier > to pass it in via the options than adding another parameter. > 1. it doesn't require changing ClangdServer interfaces, and the dynamic index > should be built by ClangdServer and thus doesn't make sense to be a parameter. We do have it as a parameter to `codeComplete` method, the fact that it's wrapped in a struct does not make much difference. `ClangdServer` should probably accept it in a constructor instead if we want to override some stuff via the dynamic index instead. > 2. it enables unit tests to use customized dummy indexes. unit-testing code can easily wrap any interface, this shouldn't be a problem. > 3. we might take another (static) index in the future, and it seems easier to > pass it in via the options than adding another parameter. I'm looking at CodeCompleteOptions as a configurable user preference rather than a struct, containing all required inputs of codeComplete. I don't think SymbolIndex is in line with other fields that this struct contains. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41281 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
