ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdUnit.h:132
-/// Manages resources, required by clangd. Allows to rebuild file with new
-/// contents, and provides AST and Preamble for it.
-class CppFile {
+/// A helper class that handles building preambles and ASTs for a file. Also
+/// adds some logging.
----------------
sammccall wrote:
> This may be change aversion, but I'm not sure this class does enough after
> this change - it doesn't store the inputs or the outputs/cache, so it kind of
> seems like it wants to be a function.
>
> I guess the motivation here is that storing the outputs means dealing with
> the cache, and the cache is local to TUScheduler.
> But `CppFile` is only used in TUScheduler, so we could move this there too?
> It feels like expanding the scope more than I'd like.
>
> The upside is that I think it's a more natural division of responsibility:
> `CppFile` could continue to be the "main" holder of the
> `shared_ptr<Preamble>` (which we don't limit, but share), and instead of
> `Optional<ParsedAST>` it'd have a `weak_ptr<ParsedAST>` which is controlled
> and can be refreshed through the cache.
>
> As discussed offline, the cache could look something like:
> ```
> class Cache {
> shared_ptr<ParsedAST> put(ParsedAST);
> void hintUsed(ParsedAST*); // optional, bumps LRU when client reads
> void hintUnused(ParsedAST*); // optional, releases when client abandons
> }
>
> shared_ptr<ParsedAST> CppFile::getAST() {
> shared_ptr<ParsedAST> AST = WeakAST.lock();
> if (AST)
> Cache.hintUsed(AST.get());
> else
> WeakAST = AST = Cache.put(build...);
> return AST;
> }
> ```
I've reimplemented the cache with weak_ptr/shared_ptr.
With a slightly different interface to hide more stuff in the cache API. Please
take a look and let me know what you think.
Nevertheless, I still find `CppFile` refactoring useful. It unties preambles
from the ASTs and I believe this is a good thing, given that their lifetimes
are different.
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
