Geoff the Medio wrote:Dilvish wrote:...taking out hte ampersand in line 80, the const_value_iterator constructor "const_iterator& base" --> "const_iterator base" .
What happens if you make ObjectMap.h line 80:
Code: Select all
const_value_iterator(const typename std::map<int, T*>::const_iterator& base) :
? I don't see why it shouldn't be possible to pass the parameter by reference.
My suspicion is that it's taking a map<int, T*>::iterator, and converting it to a map<int, T*>::const_iterator, thus creating a new temporary variable which is then being passed to the constructor.
Such conversions are probably the cause of some of the other issues we're having as well - for instance, why can't we return an std::pair<int, const T*>& by reference by returning std::map<int, T*>::const_iterator::operator*()? After all, std::map<int, T*>::const_iterator*() is obviously returning by reference. The reason is that it's implicitly converting the const std::pair<const int, T*> to a
new local variable of type std::pair<int, const T*>, and attempting to return it by reference leads to disaster.
There are two possibilities that I like - one is to override the increment and decrement operators for the const_iterator to store a version of the pair as an std::pair<int, const T*>, then return a reference or pointer to it in the * and -> operators (seems to be working - and I made sure to include a call to post-increment somewhere, just to instantiate that template function and make sure it compiles/functions correctly). We definitely need to have an intermediate variable somewhere - the question is really whether it's a local variable that we hack into the type we want when we want to return a pointer to it, or a persistent variable that we store as the type we want, and can return a pointer to without difficulty.
The second possibility (the Gordian approach) is to get rid of iterator and const_iterator completely. The value iterators (which could then be renamed iterator and const_iterator) should suffice entirely for anything anyone outside the object map wants to do, since the object's ID is part of the object itself. For stuff within the object map that requires the key, there's plain old std:
:iterator/const_iterator. This is a bit more work, and I haven't tried/probably won't try it out immediately...
As for the the issue with not being able to convert from map iterators to object map iterators, I agree that making explicit constructor calls and just using the plain old map iterator where appropriate as Geoff has done is most appropriate.
As for this issue:
Dilvish wrote:-- two instances of "conversion of scalar value_iterator to non-scalar type const_value_iterator ", triggered by Empire.cpp 1776 and 2597
Neither of these things exactly jive with the error message you're getting, but two perhaps related things:
1. A call to begin_values on a non-const ObjectMap will return an ObjectMap::iterator, which you're trying to assign to a variable of type ObjectMap::const_iterator (which should work...?).
2. Geoff's new implementation of the templated begin and end methods are actually calling the const_iterator's constructor (revealing another flaw in my design - a const_iterator is implicitly convertible to an iterator!)
Aside from the static casts and the #2 issue above, another thing I noticed about that patch was that the -> operator is overloaded to return the same type as the * operator, whereas it would typically return a pointer to that type (which is admittedly not useful in this case - hence the semantics I had in place of treating it as an iterator over a collection of T, which can be treated as a T*. Now it can be treated as a T* in some cases, and a T** in others...).
I'm putting the finishing touches on a patch that should hopefully solve everyone's problems. If it fails, feel free to commit Dilvish's updated version of Geoff's patch while I continue to work on a solution that doesn't involve hacky static casts.