[Spice-devel] [PATCH v2 0/6] sound cleanups

[Jonathon Jongsma]

On Tue, 2017-04-25 at 12:04 +0200, Christophe Fergeau wrote:
> On Tue, Apr 25, 2017 at 09:39:26AM +0200, Pavel Grunt wrote:
> > Hi Jonathon,
> > 
> > I don't see a benefit of removing ("inlining") on_new_{playback,
> > record}_channel_client. I actually prefer shorter functions even if
> > it
> > means that some other function is called only once. It is also
> > recommended in the style document:
> > https://www.spice-space.org/spice-project-coding-style-and-coding-c
> > onv
> > entions.html#_short_functions
> > 
> > In case you do it to make more obvious that the client is not
> > active
> > then imho is better to assert it.
> 
> In this case, the splitting of some of the constructor code in a
> separate on_new_XXX() make little logical sense. Why is this specific
> code in a separate function? The code which ended there seems very
> random to me. I agree that the constructor gets a bit big with
> everything in it, but in my opinion it's better than having a very
> arbitrary on_new_XXX() method. Maybe the constructor can be
> meaningfully
> split in smaller methods, but then they would probably be named
> differently.
> 
> Christophe


I agree. It feels very arbitrary that this code is split off to a
separate function. Perhaps if the name of these functions was more
specific (playback_channel_client_initialize_commands()?), it might
make more sense. But the functions don't actually do much (especially
after removing the dead code in the later patch).

Jonathon