Opened 13 years ago

Closed 11 years ago

Last modified 6 years ago

#9414 closed patch (outdated)

reverse iterators for Common::List

Reported by: SF/mobber Owned by: bluegr
Priority: normal Component: --Other--
Version: Keywords:
Cc: Game:

Description

reverse_begin() will now provide actual reverse bidirectinal iterators and constant iterators. Itertors and reverse iterators are equal if they point to the same element. The existing implementation where reverse_begin() provieds an forward bidirectinal iteratoris keept as legacy_reverse_begin() als existing uses of reverse_begin() have been changed accordingly.

Ticket imported from: #3515308. Ticket imported from: patches/1519.

Attachments (1)

Common_List_Reverse_Iterator.patch (41.5 KB ) - added by SF/mobber 13 years ago.
git format-patch

Download all attachments as: .zip

Change History (12)

comment:1 by SF/mobber, 13 years ago

I'llmake a proper patch file soon

comment:2 by SF/mobber, 13 years ago

This patch is against scummvm from git

comment:3 by SF/mobber, 13 years ago

This is a patch against the scummvm master tree: commit d50e34c1bd1152170737bea6bd85c08566426eb6

comment:4 by SF/mobber, 13 years ago

Summary: reverse iterators for Common::L:istreverse iterators for Common::List

comment:5 by SF/mobber, 13 years ago

Also added reverse_end() witch has the same function as end()

comment:6 by lordhoto, 13 years ago

I would like to take this as an opportunity to ask you again: Please submit this as a pull request on github, this will allow for easier review and thus hopefully will give you more feedback.

comment:7 by lordhoto, 13 years ago

A few notes in random order:

1) You should really read up on our http://wiki.scummvm.org/index.php/Commit_Guidelines. The commit messages look like they don't follow them as far as I can tell. 2) It seems your changes don't follow our http://wiki.scummvm.org/index.php/Code_Formatting_Conventions. Please adapt your changes accordingly. 3) It's unclear why you add commented code for ReverseIterator and ConstReverseIterator and then remove it in the next commit, it would be nice to hear about the reasons here. 4) The direction flag in the Iterator class really doesn't seem the best solution to me. Did you consider adding something similar to reverse_bidirectional_iterator from the STL and use this instead?

comment:8 by SF/mobber, 13 years ago

Answer 1. I did forget that. 2. I tried to work without a pretty printer. 3. That was somthing I tried, but decided for the simpelest solution possible, did put it into a commit thought because I consideed to finish it insted of the solution I did choose. 4.I did consider it, but I decided to do this as simple as possible, because it has been a while for me coding in c++. I am aware that it is a naive implementation, of a sort that in general is not th best. However it is absulutly suitable except for wasting performanc unnecessarily as long as it is not optimised. Space one Byte per Iterator, witch is in my opinion neglectable, because no one will generate lots of Iterators. Computation Time: 1 conditional jump per iterator iteration, this generally should take roughly one cpu cycle in effect doubleing the executiuon time. Further performance penalty can arise on a machined with a less capable brach prediction unit. This is bad but not so bad because iteration on an iterator will be done quite often, but the performace offerhead is still somewhat acceptable, althoughtit it would be not good enought for a general purpose library, witch is supposed to be highly optimsed. However a good compiler could probably optimize to code to the same performance, and about the same Machine Code, that the correct solution, witch of course is to derive a class from Iterator and to overwrite its methods, would give. Lastly I did want to write working code first, then optimize it. generallly I would not rewrite working code for performance, unless it is actually a bottelneck.

I did not do a pull request,because, I was unsure of how to do it, and asked somone in the irc chat.

by SF/mobber, 13 years ago

git format-patch

comment:9 by bluegr, 11 years ago

Owner: set to bluegr
Resolution: outdated
Status: newclosed

comment:10 by bluegr, 11 years ago

The updated version of this (which has been closed) is here: https://github.com/scummvm/scummvm/pull/272

Closing this earlier version

comment:11 by digitall, 6 years ago

Component: --Other--
Summary: reverse iterators for Common::List reverse iterators for Common::List
Note: See TracTickets for help on using tickets.