ioeric added inline comments.
================ 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 ---------------- ilya-biryukov wrote: > 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. > Thanks for the feedback! I added a FIXME for this. I am happy to clean this up when we have a saner way to config code completion in clangd :) 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
