#8749 closed patch
COMMON: Common::HashMap iterator rework
| Reported by: | lordhoto | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | --Other-- |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hi,
this patch changes the behaviour of Common::HashMap iterators to match std::map iterators, it also changes all client code accordingly.
I assign this to Fingolfin, since I was not sure if we want it to be this way for Common::HashMap, at least it helps people familiar with std::map to use Common::HashMap.
Ticket imported from: #1837119. Ticket imported from: patches/854.
Attachments (2)
Change History (12)
by , 17 years ago
| Attachment: | map_iterator.patch added |
|---|
by , 17 years ago
| Attachment: | map_iterator2.patch added |
|---|
comment:1 by , 17 years ago
I don't agree with the change of "_key" and "_value" to "first" and "second". While this might be appealing to people who are used to STL, we are not trying to exactly clone STL. The names "key" and "value" are much more natural when dealing with maps/dictionaries, I feel. We can discuss dropping the leading underscore, though.
Attached a reduced patch which omits this part. It would be nice if you could now post an explanation of what your changes do and what they are good for? I.e. which problem do they solve? File Added: map_iterator2.patch
comment:2 by , 17 years ago
Apart from the renaming, the patch added support for non const iterators. This isn't of any use currently, but it completes our HashMap implementation a bit more.
side note: I'm currently planning to rework the Kyra resource manager for better performance, since it seems like NDS has problems sometimes with the performance, and non const iterators would be of use there, maybe it'll work without though, still I think adding non const iterator shouldn't add too much bloat to our code and maybe can be of use in the future.
comment:3 by , 17 years ago
I agree, the non const iterator support would be nice. Problem is, it causes tons of warnings for me as it is. Essentially, the "Iterator(const Iterator<Node> &iter)" tries to access the _idx / _hashmap members of iter, which it is not allowed to do, since those are from another class (Iterator<NodeType>).
A quick hack to fix this hack would be to make those two members public, but that's a nasty solution. Can you do better?
comment:4 by , 17 years ago
The patch works fine for me with gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)
it does not work fine with Apple's GCC on 10.4... hrm
comment:6 by , 17 years ago
Yeah, seems like a compiler bug. But sadly that makes the patch unaccapteble for me for the time being.
comment:7 by , 17 years ago
Hm what about adding a hack to make _idx and _hashmap public on OS X? Not a nice solution, but at least it would work around the compiler problems :-).
comment:9 by , 17 years ago
| Status: | new → closed |
|---|
comment:10 by , 6 years ago
| Component: | → --Other-- |
|---|
STL like iterators for Common::HashMap