On Fri, Apr 4, 2014 at 7:28 PM, chris beck <[email protected]> wrote: > > With regards to the reference counting / possible memory corruption: The > unit_map does indeed have some reference counting built in, but that is > only for unit_map::iterator objects afaict. Many of the wesnoth modules > currently interact with the unit_map by using "extract", which extracts a > naked pointer to the unit and "transfers ownership" to the recipient. From > what I understood, the whiteboard in some circumstances takes pointers like > this and wraps them in boost shared pointers for its own use. But I guess > that this means we have two disjoint reference counting systems, so there > could potentially still be pointer problems between modules. E.g. what > happens if a unit is owned by the unit_map and being animated, but during > the animation it is transferred to the whiteboard / some other module... ? > Since animations don't use the reference counted pointer but rather a raw > pointer I think it may result in segfault if the whiteboard then destroys > the unit. (It's also worth noting that the unit_map has been failing its > unit tests regarding invalidating iterators that are supposed to be dead, > so this might also somehow be a contributing factor in some scenarios.) > I took a look at this, and there are only 3 systems where unit_map::extract is called. -Lua bindings. It is called by wesnoth.extract_unit, which removes a unit from its current owner and stores it in a garbage-collected lua object. -In unit.cpp, various temporary_unit_foo classes extract a unit, store it in a raw pointer, and put it back in their destructor. -Three whiteboard actions, which like the temporary_unit_foo classes, make modifications to various gamestate objects. Unlike the temporary_unit_foo classes though, they store it in a std::auto_ptr<unit>. This pointer seems to be non-NULL whenever the unit_map does not own the unit. The action is stored, wrapped in a shared_ptr, in some multi_index boost structure, so I'm quite unsure about object lifetimes here.
Note that the actual pointer can also be (and in certain places, is) extracted by doing &*itor. A possible solution that I mulled over was to have different modules pass > unit_pods around (the class used internally by unit_map as a > reference-counted pointer struct) or trying to make some of them use the > unit_map iterator type rather than raw pointers to units that they do not > own. But finally I concluded that since I'm not a real C++ programmer, > anything I do here would most likely be flawed and need to be redone -- > really IMO we should get a bright GSoC student who knows a lot about STL to > figure out a modern smart pointer system we can use and implement it. > As long as you take ownership of the object, you can wrap it however you like. The unit_map's unit_pod has the purpose of preventing iterator invalidation. The pod refcounts the iterators and is only removed from the map when there are no other references to it remaining. Regarding unit animations. They indeed take a raw pointer that is owned by someone else and assume the unit continues to exist during the length of the animation. Furthermore, animations use state *owned by the unit*, so they do not support reentrancy. (see bug #18921) >
_______________________________________________ Wesnoth-dev mailing list [email protected] https://mail.gna.org/listinfo/wesnoth-dev
