On 12/20/19 2:31 PM, Daniel Henrique Barboza wrote: > > > 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 > Ah I didn't realize (or forgot). I'll defer to Jano on this one, he's done far more reviews than me. So I suggest sticking with his method Thanks, Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
