Opened 12 years ago

Closed 11 years ago

Last modified 7 months ago

#8749 closed patch

COMMON: Common::HashMap iterator rework

Reported by: lordhoto Owned by: fingolfin
Priority: normal Component: --Other--
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 12 years ago.
STL like iterators for Common::HashMap
map_iterator2.patch (3.3 KB) - added by fingolfin 12 years ago.

Download all attachments as: .zip

Change History (12)

Changed 12 years ago by lordhoto

Attachment: map_iterator.patch added

STL like iterators for Common::HashMap

Changed 12 years ago by fingolfin

Attachment: map_iterator2.patch added

comment:1 Changed 12 years ago by fingolfin

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 Changed 12 years ago by lordhoto

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 Changed 12 years ago by fingolfin

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 Changed 12 years ago by fingolfin

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 Changed 11 years ago by bluegr

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

comment:6 Changed 11 years ago by fingolfin

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

comment:7 Changed 11 years ago by lordhoto

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 Changed 11 years ago by fingolfin

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

comment:9 Changed 11 years ago by fingolfin

Status: newclosed

comment:10 Changed 7 months ago by digitall

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