breese / trial.protocol

Network wire protocols
11 stars 4 forks source link

Ordered or unordered? #12

Closed breese closed 7 years ago

breese commented 7 years ago

dynamic::variable supports associative arrays. This is currently implemented with std::map for convenience - I was too lazy to implement a hash function for variable.

I deliberately kept the variable iterators as ForwardIterator so we would be able to switch the implementation to std::unordered_map.

While writing test suites that uses std algorithms, I encountered several that cannot be used with variable because of its iterator category. For instance, we can use std::is_sorted (requires ForwardIterator) but not std::sort (requires RandomAccessIterator.)

The interesting algorithms are std::partition (requires BidirectionalIterator) and std::sort. Lack of support for std::partition is not a major problem because we can get a partition with std::remove. Support for std::sort is more interesting, because although it makes no sense on an associative array (which is either sorted by key, or unsortable) but it would be nice to be able to sort the elements in a normal array.

So the question is, shall variable::iterator model BidirectionalIterator/RandomAccessIterator or continue to model ForwardIterator? The former will give us access to more algorithms, but we lose the ability to use std::unordered_map.

I have slight preference for ForwardIterator, because reordering algorithms, e.g. std::sort and std::reverse, may have a strange effect on associative arrays.

vinipsmaker commented 7 years ago

From what I understand, there is a precondition to call dynamic::variable::begin. User can't call this function on int-valued variables. Only map and array types make sense in this method.

If there are plans to change this behaviour (make other types behave like a 1-sized array collection in regards to iterators), I'll have to think more about this problem.

In the other case, I think it's worth to just create two iterators and make access to them using two different functions (begin_array, begin_map...). I mean, user already has to know the type, so what's the concern here? A separate begin function could unify all types (and not only array and map, but also int-valued and others to behave like a 1-sized col...).

breese commented 7 years ago

All types can be iterated over today. They are regarded as:

vinipsmaker commented 7 years ago

So the question is, shall variable::iterator model BidirectionalIterator/RandomAccessIterator or continue to model ForwardIterator?

I prefer the latter.

breese commented 7 years ago

Ok, I will continue with ForwardIterator.

Fortunately, we can change our minds later on without breaking backwardscompatibility.