fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> slaveinterface.cpp:102
> + // Note for future reference: A list is maintained for sizes and times.
> + // Minimum list size is 1 and maximum list size is 8. Delta is calculated
> + // using first and last item from the list.
This comment should refer to `max_count` instead of hardcoding the number.
> slaveinterface.cpp:111
>
> + const SlaveInterfacePrivate::TransferInfo first =
> d->transfer_details.first();
> + const SlaveInterfacePrivate::TransferInfo last = {elapsed_time,
> (d->filesize - d->offset)};
This is undefined behaviour if the vector is empty.
> chinmoyr wrote in slaveinterface.cpp:113
> I doubt it will be zero here. `elapsed_time` stores elapsed time since the
> timer started so it will continue to grow (?) and the difference will always
> be > 0.
Doubt is not enough - it is absolutely necessary to special case that. A div /
0 is an instant crash on x86_64.
> slaveinterface.cpp:114
> + KIO::filesize_t lspeed = 1000 * (last.size - first.size) /
> (last.time - first.time);
> if (!lspeed) {
> + d->transfer_details.clear();
What does this actually test for?
> slaveinterface_p.h:62
> + };
> + QVector<TransferInfo> transfer_details;
> + QElapsedTimer elapsed_timer;
Is there any reason to use a qvector instead of a std::array/c array?
It'll only introduce more overhead.
> dfaure wrote in slaveinterface_p.h:39
> global namespace pollution, better keep this within
> KIO::SlaveInterfacePrivate.
I absolutely agree here.
There is no reason to have this as a global variable, especially not a "static"
one in a header.
Please move it into the class.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17528
To: chinmoyr, dfaure, fvogt
Cc: fvogt, davidedmundson, broulik, bruns, kde-frameworks-devel, michaelh,
ngraham
