[Spice-devel] [PATCH spice-common] Integrate recorder library

[Christophe Fergeau]

Hey,

On Thu, Nov 29, 2018 at 12:23:16PM -0500, Frediano Ziglio wrote:
> > For example, when Marc-André added the json-glib dependency (
> > https://lists.freedesktop.org/archives/spice-devel/2018-August/045202.html
> > ), we did not get first a patch adding the new dep and not doing
> > anythingelse, and the rest of the series after this initial patch was
> > merged. The addition of the new dep was sent at the same time as the
> > patches which need json-glib.
> > 
> 
> Usually following patches are also an explanation of the stuff you are
> adding. In this case the usage, as you also said, is pretty clear.

It's clear it's going to be used to gather statistics, exactly how it
would look, I have no idea. And I don't understand why there are no
statistics gathering patches coming with it. Such patches don't even
require the recorder to come first, as the recorder macros could
probably temporarily use g_log or g_log_structured. So I don't really
see a reason for not having one or two accompanying patches
illustrating how it's going to be used.


> A difference of this patch is that is not a simple library addition,
> there's an option, a dummy replacement, a test and configure function.

Yes, a separate patch for adding the recorder is good given it's a non
trivial change (wrt the size of the patch).

> What you probably want to say is "I'd prefer the merge to be done
> with some code really making use of this library".

Yes, that's it, though it's not just the merge, but even for the review.
Easier to comment about a new internal API when you can see how it's
used.

> If that the case however should not be an excuse to avoid to review it
> and discuss.

Regarding reviewing the dependency addition, I don't think I had a lot
to say about the patch itself (I can take a look again).

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181130/234bd092/attachment-0001.sig>