D17528: Refactor SlaveInterface::calcSpeed

[Fabian Vogt]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190114/35cfe190/attachment.html>