Opened 16 years ago

Closed 16 years ago

Last modified 5 years ago

#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)

map_iterator.patch (13.8 KB ) - added by lordhoto 16 years ago.
STL like iterators for Common::HashMap
map_iterator2.patch (3.3 KB ) - added by fingolfin 16 years ago.

Download all attachments as: .zip

Change History (12)

by lordhoto, 16 years ago

Attachment: map_iterator.patch added

STL like iterators for Common::HashMap

by fingolfin, 16 years ago

Attachment: map_iterator2.patch added

comment:1 by fingolfin, 16 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 lordhoto, 16 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 fingolfin, 16 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 fingolfin, 16 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:5 by bluegr, 16 years ago

map_iterator2.patch works fine for me with MSVC9 (Visual Studio 2008)

comment:6 by fingolfin, 16 years ago

Yeah, seems like a compiler bug. But sadly that makes the patch unaccapteble for me for the time being.

comment:7 by lordhoto, 16 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:8 by fingolfin, 16 years ago

Aye! So I just commited this with the Mac OS X specific workaround.

comment:9 by fingolfin, 16 years ago

Status: newclosed

comment:10 by digitall, 5 years ago

Component: --Other--
Note: See TracTickets for help on using tickets.