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

[Katya Romanova via Phabricator via llvm-commits]

kromanova added inline comments.


================
Comment at: include/llvm/LTO/legacy/ThinLTOCodeGenerator.h:154
   void setCachePruningInterval(int Interval) {
-    if (Interval)
-      CacheOptions.Policy.Interval = std::chrono::seconds(Interval);
+      static_assert(std::is_same<decltype(CacheOptions.Policy.Interval),
+          std::chrono::seconds>::value,
----------------
Ben, how does this assert avoids risk of overflow? I thought this check is only to make sure that CacheOptions.Policy.Interval has type std::chrono::seconds.  

"Interval", which is a member of struct CachePruningPolicy, has type std::chrono::seconds. If we write something to Interval, the value will be implicitly converted to std::chrono::seconds, so the assert will never be triggered. Am I wrong?

Or are you adding this assert simply to prevent someone from changing the type of Interval in CachePruningPolicy without paying attention to this functionl? In this case, I still confused about "risk of overflow" comment.


================
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());
----------------
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.  




https://reviews.llvm.org/D41231