[Lldb-commits] [PATCH] D50620: Added test for Core/Range class.

[Vedant Kumar via Phabricator via lldb-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Thanks, looks good with nitpicks.



================
Comment at: unittests/Core/RangeTest.cpp:139
+  RangeT r;
+  // FIXME: This is probably not intended.
+  EXPECT_TRUE(r.ContainsEndInclusive(0));
----------------
Yeah, this looks wrong.


================
Comment at: unittests/Core/RangeTest.cpp:177
+  EXPECT_TRUE(r.Contains(RangeT(3, 0)));
+  EXPECT_TRUE(r.Contains(RangeT(4, 0)));
+  EXPECT_FALSE(r.Contains(RangeT(8, 0)));
----------------
Yep, the first two results are at least inconsistent with the third, and look wrong to me.


================
Comment at: unittests/Core/RangeTest.cpp:250
+
+// For less than, we should
+TEST(RangeTest, LessThan) {
----------------
Drop this comment?


================
Comment at: unittests/Core/RangeTest.cpp:253
+  RangeT r(10, 20);
+
+  // Equal range.
----------------
Might help to have a helper lambda here, like `expectOrderedLessThan = [](r1, r2) { expect_true(r1 < r2); expect_false(r2 < r1); }`.


https://reviews.llvm.org/D50620