[PATCH] D41231: [Support][CachePruning] Fix regression that prevents disabling of pruning

[Katya Romanova via Phabricator via llvm-commits]

kromanova added inline comments.


================
Comment at: lib/Support/CachePruning.cpp:158
   sys::fs::file_status FileStatus;
-  const auto CurrentTime = system_clock::now();
+  const auto CurrentTime =
+      time_point_cast<decltype(Policy.Interval)>(system_clock::now());
----------------
kromanova wrote:
> Ben, 
> In this conversion (and in the one below) you explicitly convert std::chrono::nanoseconds (system_clock::now()) to std::chrono::seconds
> (Policy.Interval), and as a result you have CurrentTime in seconds. You do a similar explicit conversion to have TimeStampModTime (line 172) and TimeStampAge (line 174) in seconds, in order to avoid the overflow on the line 175, where Policy.Interval() could overflow when there is a max value stored in it, since "max" seconds will be converted to nanoseconds for comparison. Is my understanding of your "overflow prevention" right?
> 
> I personally think that these conversions make the code harder to read, especially a few lines later there is a similar conversion happening, but since there is no danger of overflow, you didn't change it. 
> const auto FileAccessTime = StatusOrErr->getLastAccessedTime();
> These inconsistences and tricks with conversions to avoid overflow make the code harder to understand.
> 
> Have you considered Pavel's suggestion about setting the interval to std::chrono::*nanoseconds*::max()? It's plenty sufficient, and we won't need explicit conversions.  
> 
> 
Or maybe instead of using max(), you could define your own constant (in seconds), that is large enough to ensure that pruning won't happen in the next century, but won't overflow when converted to nanoseconds? 

Or (I know already I will be hanged for his suggestion) to add one Boolean member to CachePruningPolicy struct, that will be set to false if we don't want ever run the pruner.  


================
Comment at: test/ThinLTO/X86/cache.ll:38
 ; Verify that enabling caching is working with llvm-lto2
 ; RUN: rm -Rf %t.cache
+; RUN: llvm-lto2 run -o %t.o %t2.bc %t.bc -cache-dir %t.cache \
----------------
This is not directly related to Ben's review, I was just wondering why here, for llvm-lto2 we don't test that the pruner only removes files matching the pattern "llvmcache-*" how we do in the check just above with llvm-lto?


https://reviews.llvm.org/D41231