ilya-biryukov added a comment.
Thanks for the cleanups, mostly NITs from my side.
================
Comment at: clangd/ClangdServer.cpp:81
- SymbolIndex &index() const { return *MergedIndex; }
+ SymbolIndex &index() { return *MergedIndex; }
----------------
Maybe return `const SymbolIndex&` and keep the method const?
================
Comment at: clangd/ClangdServer.cpp:101
+ };
+ return llvm::make_unique<CB>(CB{this});
+ };
----------------
Maybe simplify to `make_unique<CB>(this)`? This should work, right?
================
Comment at: clangd/ClangdServer.cpp:122
+ // - symbols declared both in the main file and the preamble
+ // (Note that symbols *only* in the main file are not indexed).
FileIndex MainFileIdx;
----------------
I thought it was not true, but now I notice we actually don't index those
symbols.
I think we should index them for workspaceSymbols, but not for completion. Any
pointers to the code that filters them out?
================
Comment at: clangd/ClangdServer.cpp:152
WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
- DynamicIdx ? *DynamicIdx : noopParsingCallbacks(),
+ DynamicIdx ? DynamicIdx->makeUpdateCallbacks() : nullptr,
Opts.UpdateDebounce, Opts.RetentionPolicy) {
----------------
Any reason to prefer `nullptr` over the no-op callbacks?
Might be a personal preference, my reasoning is: having an extra check for null
before on each of the call sites both adds unnecessary boilerplate and adds an
extra branch before the virtual call (which is, technically, another form of a
branch).
Not opposed to doing it either way, though.
================
Comment at: clangd/ClangdServer.h:195
+ /// This can be useful for testing, debugging, or observing memory usage.
+ const SymbolIndex *dynamicIndex();
+
----------------
Maybe make it const?
================
Comment at: clangd/TUScheduler.cpp:632
bool StorePreamblesInMemory,
- ParsingCallbacks &Callbacks,
+ std::unique_ptr<ParsingCallbacks> Callbacks,
std::chrono::steady_clock::duration UpdateDebounce,
----------------
Why not `ParsingCallbacks*` or `ParsingCallbacks&`? This restricts the lifetime
of the passed values.
Our particular use-case LG this way, but it seems there is no reason why
`TUScheduler` should own the callbacks.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51221
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
