Review Request: Statistics Synchronization: final review request

[Edward Hades Toroshchin]

On Sun, Nov 18, 2012 at 10:30:11PM -0000, Matěj Laitl wrote:
> Well, the method is (and should only be) called by existing
> StatSyncing code

Then the existing code is the user of the API. The code exists, doesn't
mean it will stay unmodified for the rest of times.

> the "users" of the API would be implementers of StatSyncing::Provider,
> thus the docs are targeted towards them.

Implementers of Provider aren't "users" of API, but "implementers" of
API.

> Well, I haven't tested that QNetworkReply works without an event loop,
> so perhaps it does matter. On the other hand, since the code emits the
> signal from other thread Qt::QueuedConnection is used implicitly and

You contradict yourself.

> I've dropped the Qt::QueuedConnection for your satisfaction. ;)

It's not for my satisfaction, it's for consistency. In similar parts
of the code you use different types of connections, for no reason.

> I believe that looping on isFinished() would be actually uglier,
> because waitForReadyRead() may fire multiple times and the Last.fm may
> require multiple requests.

It would be ugly, but I wouldn't say it would be uglier. But okay, I'll
take it.

> Plus the QSemaphore usage here is a clean producer-consumer pattern
> here, you must have learnt it in your CS class. Or perhaps not, but
> the code works well and IMO is not really convoluted, no motivation to
> change it on my side.

Well, if you document the diamond inheritance for people who have heard
first of it, you could've documented this "pattern" here as well.

-- 
Edward "Hades" Toroshchin
dr_lepper on irc.freenode.org