[Lldb-commits] [PATCH] D131531: [lldb] Allow DataFileCache to be constructed with a different policy

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

In D131531#3711618 <https://reviews.llvm.org/D131531#3711618>, @clayborg wrote:

> Is this new constructor going to be used elsewhere? It would be great to get more description on why this is desired/needed?

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.



================
Comment at: lldb/source/Core/DataFileCache.cpp:23-39
   // Prune the cache based off of the LLDB settings each time we create a cache
   // object.
   ModuleListProperties &properties =
       ModuleList::GetGlobalModuleListProperties();
   llvm::CachePruningPolicy policy;
   // Only scan once an hour. If we have lots of debug sessions we don't want
   // to scan this directory too often. A timestamp file is written to the
----------------
JDevlieghere wrote:
> 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.
Sounds like a good idea!


================
Comment at: lldb/source/Core/DataFileCache.cpp:40
       std::chrono::hours(properties.GetLLDBIndexCacheExpirationDays() * 24);
   pruneCache(path, policy);
+  DataFileCache(path, policy);
----------------
JDevlieghere wrote:
> Is it intentional that we now call this twice?
No! Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131531/new/

https://reviews.llvm.org/D131531