[PATCH] D31020: Support: Add a cache pruning policy parser.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked an inline comment as done.
pcc added inline comments.


================
Comment at: llvm/include/llvm/LTO/Caching.h:23
+
+struct CachePruningPolicy;
+
----------------
mehdi_amini wrote:
> Do all the clients of this header need this declaration? I found strange that nothing changes in the header but this is needed.
This was an unintentional change, I've reverted it.


================
Comment at: llvm/lib/Support/CachePruning.cpp:37
 
+static Expected<std::chrono::seconds> parseInterval(StringRef Interval) {
+  if (Interval.empty())
----------------
mehdi_amini wrote:
> Why "Interval"? It seems that you're parsing a "time" or a "duration"?
They're synonyms for the most part, but it looks like "duration" is the term used by the stdlib and distinguishes from the pruning interval, so I've switched to it.


================
Comment at: llvm/lib/Support/CachePruning.cpp:98
+                                     inconvertibleErrorCode());
+    }
+  }
----------------
mehdi_amini wrote:
> StringSwitch? I guess it does not play well with the error to return...
Right, I guess I'd have to go through contortions in this code that would outweigh the value of StringSwitch IMHO.


https://reviews.llvm.org/D31020