hokein added inline comments.
================ Comment at: clangd/index/SymbolCollector.cpp:543 + + if (!(S.Flags & Symbol::IndexedForCodeCompletion)) + return Insert(S); ---------------- ilya-biryukov wrote: > hokein wrote: > > ilya-biryukov wrote: > > > Most of the fields updated at the bottom aren't useful. However, I feel > > > the documentation is actually important, since Sema only has doc comments > > > for the **current** file and the rest are currently expected to be > > > provided by the index. > > > > > > I'm not sure if we already have the code to query the doc comments via > > > index for member completions. If not, it's an oversight. > > > In any case, I suggest we **always** store the comments in **dynamic** > > > index. Not storing the comments in the static index is fine, since any > > > data for member completions should be provided by the dynamic index (we > > > see a member in completion ⇒ sema has processed the headers ⇒ the dynamic > > > index should know about those members) > > This is a good point. > > > > For class member completions, we rely solely on Sema completions (no query > > being queried). I'm not sure it is practical to query the index for member > > completions. > > - this means for **every** code completion, we query the index, it may slow > > down completions > > - `fuzzyFind` is not supported for class members in our internal index > > service (due to the large number of them) > > > > So it turns two possibilities: > > > > 1) always store comments (`SymbolCollector` doesn't know whether it is used > > in static index or dynamic index) > > 2) or drop them for now, this wouldn't break anything, and add it back when > > we actually use them for class completions > > > > I slightly prefer 2) at the moment. WDYT? > Yeah, instead of using `fuzzyFind()`, we'll call `lookup()` to get details > of the symbols we've discovered. > It's true that this is going to add some latency, but I hope we have the > latency budget to handle this (these queries should be fast, e.g. we're doing > this for signature help and I haven't seen any noticeable latency there from > the index query, most of the running time is parsing C++). > > I also like option 2, but unfortunately we already rely on this to get the > comments in signature help, so this change would actually introduce > regressions there (less used than code completion, but still not nice to > break it) > Would the third option of having a config option (e.g. > `SymbolCollector::Options::StoreAllComments`) work? `clangd-indexer` and > auto-indexer would set the option to false, `indexSymbols` would set the > option to true. We'll both get the optimizations and avoid introducing any > regressions. The plumbing should be no more than a few lines of code. Sounds fair. I totally missed `signature help`, it is unfortunate that our test case doesn't find this regression (added one!) > Would the third option of having a config option (e.g. > SymbolCollector::Options::StoreAllComments) work? clangd-indexer and > auto-indexer would set the option to false, indexSymbols would set the option > to true. We'll both get the optimizations and avoid introducing any > regressions. The plumbing should be no more than a few lines of code. This is a reasonable solution, but I prefer to do it in a separated patch. Now I make this patch always store docs, which should not introduce any regressions. There is another optimization opportunity here -- unlike header symbols, docs from main-file symbols can be dropped from the index, we can retrieve them from Sema. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56314/new/ https://reviews.llvm.org/D56314 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
