Details
Diff Detail
Event Timeline
The tests seem fine, but as a matter of style I would suggest two changes:
- replace ASSERT_XXX with EXPECT_XXX: EXPECT macros will not terminate the test, so you'll be able to see additional failures, which is helpful in pinpointing the problem. ASSERT is good for cases where the subsequent checks make no sense or will crash if the previous check is not satisfied (but that is not the case for your checks AFAICT).
- replace ASSERT_TRUE(foo == bar) with ASSERT_EQ(foo, bar) (and similar for other relational operators). Right not that does not matter much, but if someone later implements operator<< for this class, you will automatically get a helpful error message describing the objects instead of a useless "false is not true" message.
- ASSERT_ -> EXPECT_
- Now using gtest's comparison macros where possible.
- Added a PrintTo function as otherwise the gtest comparison macros won't compile.
unittests/Utility/VMRangeTest.cpp | ||
---|---|---|
43 | Seems like a few more edge cases might be good: EXPECT_NE(range1, VMRange(0x100, 0x1ff)); EXPECT_NE(range1, VMRange(0x100, 0x201)); EXPECT_NE(range1, VMRange(0x0ff, 0x200)); EXPECT_NE(range1, VMRange(0x101 0x200)); | |
72 | Probably don't need the 0xFE as 0xFF will test the condition of an address before 0x100 | |
79 | Should probably test the UINT64_MAX address instead of just 0xfff as the last edge case | |
89 | add a zero sized range check where the address is inside the other: EXPECT_FALSE(range.Contains(VMRange(0x100, 0x100))); | |
92 | Add a test to see that an equal range is also contained: EXPECT_TRUE(range.Contains(VMRange(0x100, 0x200)); | |
93 | change 0x208 to 0x201 so it is just outside the range | |
93 | Add check for MAX address: EXPECT_FALSE(range.Contains(VMRange(0x105, UINT64_MAX))); | |
104 | Test ordering of empty ranges? Not sure if that makes sense |
Sorry, I did not anticipate that. It looks like the iterator typedef inside the VMRange is confusing gtest's universal printing logic (normally it does not require a type to be printable and it will just print a hex dump instead). In any case, having the PrintTo function is a good thing, so thanks for adding it.
The new test cases did catch a real bug:
[ RUN ] VMRange.CollectionContains
llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure
Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104))
Actual: false
Expected: true
class RangeInRangeUnaryPredicate {
public:
RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that
_range binds to a temporary!
bool operator()(const VMRange &range) const {
return range.Contains(_range);
}
const VMRange &_range;
};
I just sent out a review for the fix (https://reviews.llvm.org/D50290)
Seems like a few more edge cases might be good: