Add test cases for the List to cover public methods
#42136
Conversation
tests/test_list.h
Outdated
| CHECK_MESSAGE(n[2] != nullptr, | ||
| "The element should still be accessible before the list is cleared."); |
There was a problem hiding this comment.
Actually I'm not sure if that's a feature, because the element should be deleted by now, but I think that the pointer is not reset to nullptr, makes sense?
There was a problem hiding this comment.
Wait, isn't List contiguous? If you delete the 3rd element (value 2), n[2] should still be valid and point to 3 no?
There was a problem hiding this comment.
I guess the issue is that you're still using the old iterator. Maybe it should be invalidated somehow if you erase an element?
If you look at
T &operator[](int p_index) {
CRASH_BAD_INDEX(p_index, size());
...
I'd assume that if you try to read n[9] after erasing the value 2, you'll get a crash, while n[8]->next()->get() might actually be 10.
There was a problem hiding this comment.
Note that n is just an array of elements which is populated for testing purposes (through the same list.push_back() mechanism), not a list.
So, I think the pointer should still be valid and the CHECK_MESSAGE should be removed, but when checking for the actual value:
CHECK(n[2]->get() == 2);
it would still hold the erased value.
I've done some changes to List::_Data::erase() locally by resetting the memory to zero after delete:
memdelete_allocator<Element, A>(const_cast<Element *>(p_I));
zeromem(p_I, sizeof(Element));
And the above assertion fails now.
Doing so would impact performance I guess, and that's relying on undefined behavior anyway.
So, I've just added a comment for that in the test case.
| TEST_CASE("[List] Insert before null") { | ||
| List<String> list; | ||
| List<String>::Element *a = list.push_back("A"); | ||
| List<String>::Element *b = list.push_back("B"); | ||
| List<String>::Element *c = list.push_back("C"); | ||
|
|
||
| list.insert_before(nullptr, "I"); | ||
|
|
||
| CHECK(a->next()->get() == "B"); | ||
| CHECK(b->get() == "B"); | ||
| CHECK(c->prev()->prev()->get() == "A"); | ||
| CHECK(list.front()->next()->get() == "B"); | ||
| CHECK(list.back()->prev()->prev()->get() == "B"); | ||
| CHECK(list.back()->get() == "I"); | ||
| } |
There was a problem hiding this comment.
See also #42116, but this ensures that the current implementation is covered at least.
There was a problem hiding this comment.
Yeah I think this should likely be fixed.
There was a problem hiding this comment.
There are two ways to fix this:
- Modify the behavior so that
insert_before(null, value)pushes to the front of the list instead of back. - Actually handle this case as an error (for both
insert_before(null, value)andinsert_after(null, value)), because you can always check whether an element is valid beforehand and decide whether you want to insert a new value elsewhere.
Except for overloaded operators.
|
Thanks! |
Except for overloaded operators...