Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#1519 reverse iterators for Common::List

GSoC
closed-out-of-date
None
5
2013-12-02
2012-04-05
Mathias Bartl
No

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.

Discussion

  • Mathias Bartl
    Mathias Bartl
    2012-04-05

    I'llmake a proper patch file soon

     
  • Mathias Bartl
    Mathias Bartl
    2012-04-06

    This patch is against scummvm from git

     
  • Mathias Bartl
    Mathias Bartl
    2012-04-06

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

     
  • Mathias Bartl
    Mathias Bartl
    2012-04-06

    • summary: reverse iterators for Common::L:ist --> reverse iterators for Common::List
     
  • Mathias Bartl
    Mathias Bartl
    2012-04-06

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

     
  • 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.

     
  • 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?

     
  • Mathias Bartl
    Mathias Bartl
    2012-04-15

    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.

     
    • status: open --> closed-out-of-date
    • assigned_to: Filippos Karapetis
    • Group: --> GSoC