Re: [libvirt] [PATCH v4 0/4] qemu_process: Glib sponsored cleanup

On 12/20/19 3:57 PM, Cole Robinson wrote: > On 12/19/19 4:09 PM, Daniel Henrique Barboza wrote: >> This series got buried a few months ago. Let's go onward unto >> the 20s with no one left behind. >> >> changes from version 3 [1]: >> - rebased to master at commit 330b556829 >> - removed former patch 4. The 'g_strdup_printf' change was >> made along the road >> - new patch 4: I am sending this one in separate to patch 3 >> because this unneeded label was already in the code, being >> unrelated to the changes of this series. The maintainer is >> welcome to squash it into patch 3. >> >> [1] >> https://www.redhat.com/archives/libvir-list/2019-October/msg01080.html >> >> Daniel Henrique Barboza (4): >>    qemu_process.c: use g_autofree >>    qemu_process.c: use g_autoptr() >>    qemu_process.c: remove cleanup labels after g_auto*() changes >>    qemu_process.c: remove 'cleanup' label from >>      qemuProcessCreatePretendCmd() >> >>   src/qemu/qemu_process.c | 429 +++++++++++++++------------------------- >>   1 file changed, 157 insertions(+), 272 deletions(-) >> > > Patch 3 and 4 look fine, some comments on the first two. > > I can't really decide what is the best way to approach cleanups like > this. Should patches be split by function, and do all cleanups in one > pass, or do one type of cleanup, but over a larger surface per file? > Like you have done here. This cleanup strategy was proposed by Jano [1] a few months ago in another cleanup [2]. This is what he proposed: ---- These patches would look much better split by: * individual functions (in case you do rework multiple things at once) * individual changes, i.e.  * g_autofree for scalars  * g_autoptr for pointers and unref  * possible removal of cleanup labels Especially splitting the goto -> return change makes the patches much more easier to read, since it makes it obvious that you don't change the exit points of the function while adding the autofree attributes. ---- He followed up with: ---- > Do you mean sending an individual patch for any function that might > have, say, 2+ changes in it? For example, if the same function > was changed to use g_autoptr and g_autofree and perhaps > that causes a label to be dropped, this can be an individual patch? Yes, but if you convert a lot of functions, that would result in a lot of patches. Sending one patch per function is more viable for the cases where you need to refactor it in order to add some functionality later (see Jirka's series for example). For mass conversion for the sake of conversion, one patch per change is better. ---- These cleanups are cleanups for the sake of cleanups, thus I followed up with this model of one type of change across all the file per patch. [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00985.html [2] https://www.redhat.com/archives/libvir-list/2019-October/msg00918.html