ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM. I still think that we should move the `SymbolIndex` out of the struct, but don't want to block this patch.
================ 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 ---------------- sammccall wrote: > ilya-biryukov wrote: > > 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. > So yeah, this is my fault... > I do agree it's unfortunate to have a non-user-specified param in the > CodeComplete options. It's tricky - from the perspective of this module the > index really is such an option, and we want to propagate it around like one. > We just don't (currently) want to encourage its use as an API to clangdserver. > > `codeComplete` currently has 9(!) arguments. Different people will have > different reactions to this, mine is to do almost anything to avoid a 10th :-) > > > unit-testing code can easily wrap any interface, this shouldn't be a > > problem. > This doesn't match my experience at all. Can you suggest how to test this > logic without adding parameters to at least ClangdServer::codeComplete and > the codeComplete test helpers? > > > ClangdServer should probably accept it in a constructor instead if we want > > to override some stuff via the dynamic index instead. > As of today, the dynamic index is the only index clangd supports. There's > nothing to accept in the constructor, it's entirely owned by ClangdServer. > codeComplete currently has 9(!) arguments. Different people will have > different reactions to this, mine is to do almost anything to avoid a 10th :-) I do agree this is unfortunate and I'm not at all opposed to cleaning that up. By removing parameters that we don't need or grouping them into a struct that has saner default parameters or in a different way. Both 9 and 10 seem equally bad to me. My point is that `SymbolIndex` should not be part of `CodeCompleteOptions` only because it makes it easier to pass it around. > This doesn't match my experience at all. Can you suggest how to test this > logic without adding parameters to at least ClangdServer::codeComplete and > the codeComplete test helpers? Exactly, I would go with the test helper. > As of today, the dynamic index is the only index clangd supports. There's > nothing to accept in the constructor, it's entirely owned by ClangdServer. We do have implementations of the index in the tests that we pass around. That being said, I don't want this change to be blocked by my opinion on this matter. This is a minor thing, compared to all the other changes in the patch. Just wanted to make a point that this field totally feels out of place. 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
