sammccall added a comment.
Having taken a closer look, I think the cache can be simplified/separated a bit
more cleanly by returning shared pointers and not allowing lookups, instead
restoring limited ownership in CppFile...
Happy to discuss more, esp if you might disagree :)
================
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.
----------------
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;
}
```
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
