[PATCH] D63672: Added Delta IR Reduction Tool

[Diego Treviño via Phabricator via llvm-commits]

diegotf marked 7 inline comments as done.
diegotf added inline comments.


================
Comment at: llvm/tools/llvm-reduce/TestRunner.cpp:7-9
+/// RunProgramWithTimeout - This function provides an alternate interface
+/// to the sys::Program::ExecuteAndWait interface.
+/// @see sys::Program::ExecuteAndWait
----------------
chandlerc wrote:
> No need to repeat the function names.
> 
> But more importantly -- what kind of alternative? What's the difference?
True, I just grabbed that function directly from bugpoint, I hadn't noticed it's actually redundant


================
Comment at: llvm/tools/llvm-reduce/TestRunner.h:47
+private:
+  SmallString<128> TestName;
+  std::vector<std::string> TestArgs;
----------------
chandlerc wrote:
> Any particular reason to use `SmallString` instead of `std::string`?
Just trying to use ADT where possible (as per @lebedev.ri's suggestion), and since said variables are going to be Paths they will generally be under 128 chars.


================
Comment at: llvm/tools/llvm-reduce/deltas/Chunk.h:27-30
+  /// Operator used for set<Chunk>
+  friend bool operator<(const Chunk &C1, const Chunk &C2) {
+    return C1.end < C2.begin;
+  }
----------------
dblaikie wrote:
> Is this comparison sufficient for std::set (strict weak ordering - specifically I'd be worried about !({1, 4} < {3, 6}) && !({3, 6} < {1, 4}) - so perhaps this comparison should have an assertion that the two ranges are non-overlapping?) 
I actually thought about that case but since I'm just using sets for lookup (and dont really care about the order) I left the comparison as is, would you still recommend I add the assertion? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63672