#8839 closed patch
Managed containers
| Reported by: | SF/next_ghost | Owned by: | |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
I've written ManagedList and ManagedArray implementation because I'll need both of them in CinE. They are noncopyable with least code redundancy possible.
I've also modified Array::remove_at() and Stack::pop() to return void because the only return type that makes sense for ManagedArray::remove_at() is void. ManagedArray patch includes necessary changes to all parts of code where remove_at() or pop() is used (and a little performance gain as side effect of the change). These changes compile cleanly but need testing.
MoviePlayer class in Sword1 engine and CursorManager in graphics could use ManagedArray (see array2.patch for details). The classes should be ready for use in Lure and Parallaction.
I know about patch 1895703, LordHoto has seen both of my patches and agreed with posting them here.
Ticket imported from: #1906528. Ticket imported from: patches/944.
Attachments (4)
Change History (22)
by , 17 years ago
| Attachment: | list.patch added |
|---|
comment:2 by , 17 years ago
I spent some time converting Lure to use this Common::ManagedList and Common::ManagedArray where appropriate, and it went fairly smoothly - it was mostly a case of removing all the *'s I included in the instanations of my template, and which this template class includes implicitly.
The only one problem I did have turned out to be of my own making - I was getting several "deletion of pointer to incomplete type - no destructor called" using ManagedArray. I realised that this was due to a circular reference in my header files, and at the point where I was defining a ManagedArray<Hotspot>, hotspot wasn't defined. By splitting up the offending file (surface.cpp) into a seperate surface.cpp and dialogs.cpp file, I solved the error. Obviously I'll wait to commit my changes until the patch is approved.
Two suggestions I'd like to make:
* I had to move the iterator typedef in ManagedList into the public section, since derived classes in Lure rely on using it.
* Common::Array currently has a contains method. It would be worthwhile adding a copy of it to either ManagedList or, perhaps better, to the base Common::List class.
comment:4 by , 17 years ago
Some typedef fixes and new contains() member function. Inherited contains() that dreammaster wanted is in the old patch, it compares pointer which is exactly what he wants. I've added another version of contains() which compares object data instead of pointers for future use. File Added: array3.patch
comment:5 by , 17 years ago
Personally, I think that the way that ManagedList is implemented seems much better
However, concerning ManagedArray, I really don't understand why the behavior of pop() has been changed so that it doesn't return anything...
comment:7 by , 17 years ago
I'd concurr with this - it would be better to leave the method untouched in the base class (except for making it virtual), and have the ManagedArray method simply return NULL. That way none of the existing code using Common::Array needs to be changed.
comment:8 by , 17 years ago
remove_at()/pop() should return void for three reasons: 1) Performance - remove_at()/pop() results in 3 copies of data before you return back to the caller function. operator[]/pop() will result in at most one copy. 2) Exception safety - Each copy constructor call means possible exception and you can't distinguish them in any way even if you want to. operator[]/top() will not trigger the exception at all (any exception will be triggered in the caller function after operator[]/pop() returns) while remove_at()/pop() may trigger exception in 3 different places. That's why their STL counterparts also return void. 3) Returning NULL from ManagedArray::remove_at() is more of a hack than good practice.
But if you're not convinced it's a good idea, it's trivial to modify the patch.
comment:9 by , 17 years ago
Err, if you see operator[]/pop() in my previous comment, I actually meant operator[]/top().
comment:10 by , 17 years ago
Trying to trace through the rules of what gets copied where gives me a headache, so I'll take your word on it :). But I do think at the very least that the Stack template should be left untouched, since in my experience a fundamental requirement of popping an object off a stack is that you get the value that gets removed.. the Lure script.cpp patch, for example, would really be unnecessary if we leave the class as-was.
comment:11 by , 17 years ago
OK, currently we have two different approaches to managed containers: one created by LordHoto, using shared pointers, and one by next_ghost, which subclasses the current classes: Patch #1906528 - "Managed containers" and Patch #1895703 - "COMMON: Managed List"
We can't go forward with both ideas, one of them should be ultimately be chosen. Personally, I like the idea of shared pointers more... what's your opinion? (posting this in both tracker items)
comment:12 by , 17 years ago
dreammaster, all pop() member functions of STL containers return void. Using pop() in pair with top() is standard practice. Take a look for example here: http://www.sgi.com/tech/stl/stack.html
comment:13 by , 17 years ago
next_ghost: You're right; I hadn't realised that.
On the issue of whether to use a class for managing pointers versus a shared_pointer class. I guess I'll have to side with shared pointers. It's a cleaner implementation in the long term.
comment:14 by , 17 years ago
I already stated elsewhere my clear preference for shared pointers over a special ManagedList class.
As for pop vs. pop() + top() (aka: "the void pop() change"): Yeah, STL does it that way. But we ain't STL. :)
1) Performance: True, but can you name a case where this potential performance boost would be of use? 2) Exception safety: We don't and can't (for portability reasons) use exceptions (they are in fact disabled during compile time via -fno-exceptions). But I agree that it might make debugging more difficult :). 3) Returning NULL from ManagedArray::remove_at() is more of a hack than good practice: Fully agreed. Of course, using SharedPtr<>. this becomes a non-issue, too.
Still, the main reason pop() is as it is right is convenience. But of course this is a very weak argument, too. So all in all, I don't mind changing pop() to return void, but this is an issue rather orthogonal to the main point of this tracker item (and it could be submitted as a separate patch).
comment:15 by , 17 years ago
I wonder now that we got SharedPtr, do we really need this patch anymore?
comment:16 by , 17 years ago
| Status: | new → closed |
|---|
comment:17 by , 17 years ago
This patch is no longer needed, my script system implementation patch for CinE uses SharedPtr.
comment:18 by , 6 years ago
| Component: | → --Other-- |
|---|
Common::ManagedList