fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.


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

  R241 KIO


To: chinmoyr, dfaure, fvogt
Cc: fvogt, davidedmundson, broulik, bruns, kde-frameworks-devel, michaelh, 

Reply via email to