Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Is this new constructor going to be used elsewhere? It would be great to get more description on why this is desired/needed?
lldb/source/Core/DataFileCache.cpp | ||
---|---|---|
22–49 | Would it make sense to extract this into a static function say DefaultCachePruningPolicy and then have one constructor with an optional policy argument? Something like this: DataFileCache(llvm::StringRef path, llvm::CachePruningPolicy policy = DataFileCache::DefaultCachePruningPolicy); In addition to having only one constructor, the new function also provides a natural place to document the policy. | |
50 | Is it intentional that we now call this twice? |
I'm implementing a cache for swift type metadata for downstream lldb. I thought it made sense to separate its configuration from this index cache one, since we might want to enable either of them independently of the other, set different sizes, expiration dates, etc.
lldb/source/Core/DataFileCache.cpp | ||
---|---|---|
22–49 | Sounds like a good idea! | |
50 | No! Thanks. |
Added a static 'GetLLDBIndexCachePolicy' function, removed the constructor without any policies
LGTM
lldb/source/Core/DataFileCache.cpp | ||
---|---|---|
23–50 | I was worried that by caching the policy here, changing the lldb-index-cache- settings after the first DataFileCache has been created won't have any effect. But it looks like the DataFileCache is already a static/global used by the Modules, so that's already the case in practice. Maybe a follow-up patch could make that explicit in the help enable-lldb-index-cache help description. |
It's a little weird that GetLLDBIndexCachePolicy would be private, if only from a documentation point of view so that you can refer to it in the constructor.