> On Sept. 20, 2016, 12:06 p.m., Aleix Pol Gonzalez wrote: > > KTp/Declarative/messages-model.cpp, line 222 > > <https://git.reviewboard.kde.org/r/128954/diff/1/?file=477161#file477161line222> > > > > Use iterators? > > Alexandr Akulich wrote: > How to get an index from iterator? > > Aleix Pol Gonzalez wrote: > You can count in parallel without calling `QList::operator[]` on the > index. > > Alexandr Akulich wrote: > Actually I call ```const T &at(int i) const;```. And what is the reason > to use iterators, if you still suggest to use a counter? Do you actually > think that > ``` > int i = d->messages.count() - 1; > for (auto it = d->messages.constEnd(); it != > d->messages.constBegin(); --it, --i) { > if (sentTimestamp > it->message.time()) { > newMessageIndex = i; > break; > } > // Or do you suggest to place --i; here? > } > ``` > is more readable, than plain > ``` > for (int i = d->messages.count() - 1; i >= 0; --i) { > if (sentTimestamp > d->messages.at(i).message.time()) { > newMessageIndex = i; > break; > } > } > ``` > ? > > IMO it is a common practice to use iterators if you don't need index (and > just use the index otherwise). It is a bit more optimal and I don't made an > error in the condition. > > Aleix Pol Gonzalez wrote: > You can use reverse iterators: > http://doc.qt.io/qt-5/qvector.html#crbegin > > Alexandr Akulich wrote: > I know about the reverse methods and it is not any better without some > reverse adaptor in QList. > > int i = d->messages.count() - 1; > for (auto it = d->messages.crbegin(); it != d->messages.crend(); > ++it, --i) { > if (sentTimestamp > it->message.time()) { > newMessageIndex = i; > break; > } > // Or do you suggest to place --i; here? > } > > Why it is better? It reads worse and it looks like "iterators for the > iterators" without a real benefits. It does not save us from an error, > because we need the (reverse) counter. Do we have a coding convention for > this case? > > Aleix Pol Gonzalez wrote: > I'd say in general the coding convention is to use iterators whenever > possible, since we can ensure the access to the random object will be > optimal. If you really feel like this is worse, feel free to use something > else. I didn't come here to play sargeant. > > Alexandr Akulich wrote: > I'm sorry if I sounded rude or unfriendly. > > Iterators can indeed save us from the assert in QList::at(), but I think > that in this particular case the code will be more complex, harder to > understand and thus error-prone: we use reverse iterators, which we have to > increase and at the same time decrease the counter. If there would be no > beginInsertRows() call, I would be all for use iterators. > > In this case we can follow the KISS rule without performance penalty. > > By the way, I can not find anything related to iterators usage in "Qt > Coding Style", "Qt Coding Conventions" or "Frameworks Coding Style" (which > seems to be a successor of Kdelibs Coding Style). The only rule is "Don't mix > const and non-const iterators." Probably we need to update the convention(s) > with C++11 things. > > I will remove the "obvious" comment and push this change, if you agree. I > would like to upload one may be "more discussable" change, which depends on > this one. > > And anyway, thanks for the feedback!
Ok, do that. - Aleix ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128954/#review99301 ----------------------------------------------------------- On Sept. 20, 2016, 11:51 a.m., Alexandr Akulich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128954/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2016, 11:51 a.m.) > > > Review request for Telepathy. > > > Repository: ktp-common-internals > > > Description > ------- > > Implemented message sort by sent timestamp (if available). > > This fixes order of scrollback messages. > > > Diffs > ----- > > KTp/Declarative/messages-model.cpp dc1088c > > Diff: https://git.reviewboard.kde.org/r/128954/diff/ > > > Testing > ------- > > Works, fixes the issue. > > > Thanks, > > Alexandr Akulich > >
