#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)
Change History (12)
comment:1 by , 12 years ago
comment:3 by , 12 years ago
This is a patch against the scummvm master tree: commit d50e34c1bd1152170737bea6bd85c08566426eb6
comment:4 by , 12 years ago
| Summary: | reverse iterators for Common::L:ist → reverse iterators for Common::List |
|---|
comment:6 by , 12 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 , 12 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 , 12 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.
comment:9 by , 10 years ago
| Owner: | set to |
|---|---|
| Resolution: | → outdated |
| Status: | new → closed |
comment:10 by , 10 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 , 5 years ago
| Component: | → --Other-- |
|---|---|
| Summary: | reverse iterators for Common::List → reverse iterators for Common::List |
I'llmake a proper patch file soon