[PATCH] D63672: Added Delta IR Reduction Tool

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

diegotf added inline comments.


================
Comment at: llvm/tools/llvm-reduce/DeltaManager.h:21-22
+
+class DeltaManager {
+public:
+  static void runDeltaPasses(TestRunner &Tester) {
----------------
dblaikie wrote:
> Might as well make it a struct if all its members are public? Or a namespace (if it's meant as a grouping of stateless functions. Or no namespace at all and just a free (Possibly inline) function in the llvm namespace.
I think inline function is the way to go, it really doesn't make sense having it as a class when it's the only member


================
Comment at: llvm/tools/llvm-reduce/TestRunner.h:33-34
+
+  /// Runs the interesting-ness test for the specified file
+  int run(SmallString<128> Filename);
+
----------------
chandlerc wrote:
> What does the return value mean?
I've added a comment to clarify that :)


================
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:
> diegotf wrote:
> > 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? 
> The issue would be that if the ordering requirement is violated, it could break even a lookup - in the example I gave above, those two objects would be considered "equivalent", for instance.
> 
> If the Chunks are necessarily non-overlapping, perhaps it'd be simpler/accurate to only compare begin to begin (rather than begin to end)? (I'd do that and/or add the assertion)
That is a very keen observation, I'll change the comparison to begin-begin (since they can't overlap). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63672