0

I am writing a simple custom (special purpose) container and would like to allow for iteration over each element, however, avoid using iterators due to the problem of iterator invalidation.

Instead of providing a pair of iterators (via begin() and end()) I was thinking to provide a for_each method that iterates over the elements and passes them to a functor. The method would increment a counter on entry and decrement it on exit. If the counter is non-null, every time a method that would modify the container (and cause iterator invalidation) is called, it would return early, resulting in a no-op. See the following for a simple example

#include <cstdlib>
#include <cstdio>
#include <cstring>
#include <cassert>


struct FloatArray {

    float *data = nullptr;
    size_t count = 0;
    size_t niter = 0;


    float *begin() { return data; }
    float *end() { return data + count; }

    template<typename Fn>
    void
    for_each(Fn &&fn)
    {
        niter++;
        for (size_t k = 0; k < count; k++) {
            fn(data[k]);
        }
        niter--;
    }
    
    void
    push_back(float f)
    {
        if (niter) {
            /* Return early if the container is being iterated over. */
            return;
        }
        float *new_data = new float[count + 1];
        memcpy(new_data, data, sizeof(*data) * count);
        if (data) {
            delete [] data;
        }
        new_data[count] = f;
        data = new_data;
        count++;
    }
};


int
main(int argc, char **argv)
{
    FloatArray arr;

    arr.push_back(1.0f);
    arr.push_back(2.0f);
    arr.push_back(3.0f);

    arr.for_each(
        [&arr](float f)
        { 
            if (f == 1.0f) {
                arr.push_back(42.0f); // no-op
            }
            printf("%f\n", f); 
        }
    );
    for (float f : arr) {
        if (f == 1.0f) {
            arr.push_back(42.0f); // Undefined behaviour
        }
        printf("%f\n", f);         
    }

    arr.push_back(4.0);
    return 0;
}

I guess similar behavior could be achieved using iterators pointing back to the container incrementing a counter on construction and decrementing it on destruction, but is think this (and iterators in general) are way more complex to implement (and understand) than the for_each method.

One concern would be that the "iterator counter" could overflow, but I am not expecting that many nested iterations.

I guess my question is: Would using a for_each method make more sense for special purpose containers, that do not need iterators for reasons other then iterating over its elements (e.g. specifying a range, or being a reference passed to a container method), since it is more simple to implement and understand (IMO) and (more easily) avoids complexities such as iterator invalidation? Also, are there better implementations then the one described above for this kind of use case?

9
  • 3
    An API which silently chooses to not do the one thing I've asked it to do seems... unhelpful to me. Your implementation is also broken as soon as you have multiple threads. Jul 14 '20 at 4:43
  • Thanks for the comment. I agree with the first part. One could throw an exception, trigger an assert, etc. if a modifier is called during iteration. I can live with the container being not thread safe. The main point is, that I feel using something like a for_each method rather than providing iterators is better design for the situation described above. Since it seems easier to handle situations where iterators become invalid. In addition, I feel iterators add a lot of noise in form of boilerplate code for very little use.
    – scttrbrn
    Jul 14 '20 at 5:36
  • Dynamically checking for illegal states like iterator invalidation can make sense, though I would expect an exception or std::abort() when a conflict is found (this is a logic error, so it can't be reasonably recovered from). Reminds me of the RefCell type in Rust, which maintains such counters to ensure that you can't get a non-const reference while any other reference is active.
    – amon
    Jul 14 '20 at 5:39
  • 1
    @scttrbrn The point isn't just that if the iterator is called during iteration it's not thread safe. If two threads simultaneously call your for_each function, you can break your container for all time. Jul 14 '20 at 8:10
  • 3
    @scttrbrn "STL containers are not thread safe, when calling methods that modify their internal state" - Not quite - they are not thread-safe for concurrent modification operations. But they are thread-safe for read-only operations. And from the logical view of the user, your for_each is a read-only operation. It should not break when called concurrently. Jul 14 '20 at 9:41
1

Would using a for_each method make more sense for special purpose containers [...]?

for_each makes a lot of sense to me for certain containers where the design of a standard iterator doesn't map so efficiently either for CPU or programmer or both. Consider this very simple example of iterating efficiently through set bits of a bitset:

// Calls 'func(n)' for each nth bit which is set.
template <class Func>
void Bitset::for_each(Func func)
{
    size_t start=0;
    for(uint64_t bits64: all_bits64)
    {
        const uint64_t bits64 = data[j];
        if(bits64 == 0xffffffffffffffffull)
        {
             // Iterate through all 64 bits if they're all set.
             for(size_t j=0; j < 64; ++j)
                 func(start + j);
        }
        else if(bits64 != 0x0ull)
        {
             // Test each individual bit if some of the 64 bits are set.
             // Can make this even more elaborate and efficient in dense cases
             // using FFZ/FFS.
             for(size_t j=0; j < 64; ++j)
             {
                 if(bits64 & (1ull << j))
                     func(start + j);
             }
        }
        start += 64;
    }
}

Something like that can be very difficult or impossible to implement as efficiently using a flat forward iterator design without additional branching in operator++. I often wish that at least ranged-based for loops used a generic for each function of this sort behind the hood (that we can overload) over standard iterators and begin/end pairs since I have cases (along with measurements) where such for_each methods can be up to two or three times as fast when there's light work being performed in each iteration.

Iterator Invalidation

In your case though, I think merely ignoring mutation requests during traversal is even more prone to confusion and mistakes in lots of circumstances than dealing with (and documenting) iterator invalidation. At the very least I think you could assert(!"Trying to mutate the container while iterating!"); or something to this effect rather than simply ignoring such requests. I dealt with a monstrous codebase a couple of decades ago where the developers had a habit of turning erroneous requests that failed to satisfy requirements like required preconditions into no-ops, and the result after years of doing that was a bug haven so difficult to maintain where many bugs had to exist in the software to avoid causing multiple new ones. I'd strongly discourage ever coding like this where we simply ignore violations of design requirements. That can create monstrosities of a sort rivaling a codebase that ignores endless failing unit and integration tests for years on end and actually starts to come to depend on most of the tests failing to produce the desired behavior.

If you are really interested in tackling the safety of such things (along with possible thread-safety in cases where mutations would otherwise occur in parallel) and want to put in the elbow grease, I've gotten the most mileage out of using persistent data structures for such cases. They forbid structural sharing of all but immutable data/memory blocks, so things like iterator invalidation become a non-concern even in multithreading cases since the shared data is never allowed to be mutated in place. A request to make changes results in a new version of the data structure with a copy-on-write approach and forbidden from mutating anything shared that exists (like what we're iterating over, making even iteration using iterators protected from invalidation; the iterators themselves store lightweight copies using value semantics of the persistent structure to prevent invalidation rather than pointers/references).

Such structures revolving around value/copy semantics do come with their overhead, but I've found I can more than compensate for that with parallel execution by having a codebase using them where thread-safety is a no-brainer. There is also a C++ library called immer which provides such structures already implemented for you along with a very interesting presentation from the author here. I haven't looked into it in depth but I suspect it also avoids iterator invalidation.

1
  • Thanks for bringing up persistent data structures! I will have a closer look at them.
    – scttrbrn
    Nov 5 '20 at 2:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.