On Fri, Jan 30, 2015 at 10:54:33AM -0500, John Scipione wrote:
So what is the solution here? The assert only checks that the list count is
0. What if the list has a NULL hole in it as in:
Tab 1
Tab 2
NULL
Tab3
??
This should be checked when inserting items to the list, not when
iterating it. This way the bug is detected immediately instead of
at an unrelated point in the code.
If all insertions are checked against this, and it still happens, it
means the list is getting corrupted. In that case, it's better to
crash (and having no NULL check will do just that).
For example, if another thread is modifying the value, such a check like
this could potentially mask the need for locking.
I wasn't really thinking about thread locking issues although that is
another good point. Would a solution that locks access to the list before
modifying it be overkill here?
I mentioned threading issues in the ticket already. I will repeat what
I said in my message:
- If this function must only be called with some lock held, that should
be documented in a comment for the function. This way we know what to
expect. It could also check that the lock is actually held with an
assert (for example BView does a lot of "looper must be locked"
checks and will call debugger in case of misuses).
- If this function can be called without any lock held, then we must
review the code to see if the list can be accessed by multiple
threads. If so, we must add locking.
It's not possible to decide wether locking should be added without
further study of the code.
