DM-19467: Add C++ iteration to GenericMap

[kfindeisen]

This PR introduces the visitor/"iteration" mechanism originally planned for GenericMap, adds some iteration-related features to GenericMap and SimpleGenericMap, and performs some refactoring of the code added in #461 that should make GenericMap easier to use and maintain. Because there are a lot of changes, I recommend reading the PR commit by commit (and possibly skipping the two "refactor" commits entirely, as they involve pushing a lot of TMP into private code).

One design decision that I'd like feedback on involves changing GenericMap::keys from a snapshot to a view. There needs to be some API that lets GenericMap know how to iterate over its implementation class, and it's best if that API is non-throwing. I see three solutions that satisfy this requirement:

1. Make keys a view, as I've done. In C++, this forces the implementation class to maintain a vector of keys, and (if a MutableGenericMap) to keep it in sync with the "main" storage in an exception-safe way. The need for exception safety makes modifying methods more complicated than they would otherwise need to be, but this approach has the advantage of being conceptually simple.
2. Return a polymorphic iterator, whose subclass is specific to a particular GenericMap implementation. This avoids the exception-safety problems of a view and is straightforward to implement. However, to support polymorphism the iterator would need to be returned by smart pointer and passed around GenericMap code by pointer or reference, which is unC++ic (for example, the standard for loop expects iterators to be passed by value) and error-prone.
3. Create a non-polymorphic, non-template "iterator" class (though it behaves more like a Python generator) that will be created by the implementation class and used to implement GenericMap methods that need it. The "iterator" contains some storage (a boost::any or std::any memento) for the state of the iteration, as well as a callable that defines how to update the state and return the current key. The implementation class is responsible for defining the "iterator"'s initial state and the callable. This approach doesn't have the exception-safety issues of the key view and doesn't pretend to be compatible with the built-in iterator framework, but it's both unpythonic and unC++ic.

Which of these three approaches will make it easier to modify and create GenericMap subclasses in the long run?

[parejkoj]

Per instructions, I didn't really look at the two "refactor" commits, so hopefully my other comments will still make sense (done per-commit).

Is the choice of using _keyView here instead of directly using a map is to keep the faster lookup of unordered_map? Is the performance difference really worth the added complexity of having to manage two containers, instead of just using map? I think that's my biggest concern of this approach: two containers are rarely better than one.

The template magic in the new visitation is certainly non-obvious to this non-expert, but it was not entirely impenetrable. I do worry that there are only a few people in LSST who would be ok with modifying that code (I certainly wouldn't have been able to come up with this approach myself).

Given your alternate suggestions above, I'm not sure there is a perfect approach, and this way I think hides most of the scary stuff in private, so it is unlikely that others will have to worry about it. I think, but am not certain, that this approach might actually be the most understandable and provide the least confusion to those adding subclasses of GenericMap. Unfortunately, I'm not sure I can properly envision what your option 2 would look like, and option 3 just feels excessively complicated to me.

[kfindeisen]

Is the choice of using _keyView here instead of directly using a map is to keep the faster lookup of unordered_map?

I'm not sure what you mean by directly using a map (C++ maps do not expose any kind of key view, as useful as it would be). My motivation was that the original keys() function required constructing a new vector, which could throw an exception from memory allocations. Those exceptions can still be thrown in the new approach, of course, but now they'd happen as part of an insertion, which is potentially throwing anyway.

[kfindeisen]

Unfortunately, I'm not sure I can properly envision what your option 2 would look like

Something like:

protected:
    virtual unique_ptr<GenericIterator> begin() const = 0;

    void _apply(Visitor&& visitor) const {
        …
        for (auto itPtr = begin(); *itPtr != *end(); ++(*itPtr)) {
            K const& key = **itPtr;
            ...
        }
    }

There's a few ways to simplify the code (e.g., assign *itPtr to a GenericIterator&, then use that in the loop), but either way it's very unidiomatic.

Edit: just realized that option 2 can't be non-throwing, because you need to dynamically allocate the GenericIterator to get polymorphism and safe memory usage simultaneously. So it doesn't actually solve the problem I was trying to solve.

[parejkoj]

C++ maps do not expose any kind of key view, as useful as it would be

Oh, ugh. I wasn't aware of that. The rest of your rationale makes sense. I wonder if it is worthwhile writing down some of this rationale in the class docstring for GenericMap, explaining why there is an extra vector? Although not useful to users of the class, it might help those trying to understand the reason for the implementation.