ilya-biryukov added inline comments.
================ Comment at: clangd/TUScheduler.cpp:71 + + /// Update the function used to compute the value. + void update(std::function<llvm::Optional<ParsedAST>()> ComputeF); ---------------- sammccall wrote: > ilya-biryukov wrote: > > sammccall wrote: > > > I think I understand this more as "updates the value" but the value is > > > lazy... > > > > > > I find this API somewhat hard to follow, maybe just because it's > > > unfamiliar. > > > I've mostly seen cache APIs look like one of: > > > 1. `Cache(function<Value(Input)> Compute)`, `Value Cache::get(Input)` > > > 2. `void Cache::put(Key, Value)`, `Optional<Value> Cache::get(Key)` > > > 3. `Handle Cache::put(Value)`, `Optional<Value> Handle::get()` > > > > > > This one is `Slot Cache::create()`, `void Slot::update(function<Value()> > > > LazyV)`, `Value Slot::get()`. > > > > > > It's similar-ish to 3, but has 3 nontrivial operations instead of 2, and > > > the slot object is externally mutable instead of immutable, so it seems > > > more complex. What does it buy us in exchange? > > > > > > (1 & 2 work well with a natural key or inputs that are easy to compare, > > > which we don't particularly have) > > As discussed offline, now we have a simpler version that keeps > > `unique_ptr`s to idle ASTs and the clients are responsible for building the > > ASTs. > > Note that it's not a "cache" per se, so we might want a different name for > > that. > > @sammccall, you suggested to call it a pool, I find it reasonable. Should > > we name it `ASTPool` instead of `ASTCache`? > I think the name is actually fine, it's still mostly a cache. > It does have things in common with a pool, but unrelated consumers can't > share a resource, so I think that name is at least as misleading. SG, let's leave as is. ================ Comment at: clangd/TUScheduler.cpp:94 + Lock.unlock(); + ForCleanup.reset(); + } ---------------- sammccall wrote: > this line isn't actually needed right? It isn't. But it makes an important operation (destructor of the AST) explicit, so I'd still keep it. ================ Comment at: clangd/TUScheduler.cpp:342 + if (!AST) + return Action(llvm::make_error<llvm::StringError>( + "invalid AST", llvm::errc::invalid_argument)); ---------------- sammccall wrote: > This failure doesn't get cached, correct? That's bad for performance. > > But if we think this is always a clangd bug, it's probably fine. (and > certainly simplifies things) Thanks, good catch! I somehow missed it, since at some point the failure **was** cached. I think we should cache it. Failing ASTs is a clangd bug, of course. However, they might be hard to fix if it's something inside clang, so I believe we should handle the failures gracefully in that case. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47063 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
