[prev in list] [next in list] [prev in thread] [next in thread] 

List:       rpmorg-maint
Subject:    [Rpm-maint] [PATCH] Cleanup: Move similar repeating code in rpmcliArgIter to rpmcliArgIterHelper.
From:       rakesh.pandit () gmail ! com (Rakesh Pandit)
Date:       2009-03-14 12:53:16
Message-ID: b401d2530903140541k3ef9654q3b6aa31dbf93f39 () mail ! gmail ! com
[Download RAW message or body]

2009/3/13 Panu Matilainen wrote:
> On Fri, 13 Mar 2009, Rakesh Pandit wrote:

[..]

Thanks for review.

> Copy-paste elimination always welcome, just some minor issues/suggestions:
> 
> - Please keep alloc+free pairs "symmetric" when possible, eg
> ?rpmcliArgIterHelper() does rpmgiNew() so it should be freed there too,
> ?and as the default case in rpmcliArgIter() makes it's own rpmgiNew()
> ?it should also free it there. With the current patch, the
> ?lone rpmgiFree() in rpmcliArgIter() looks like "umm where did that
> ?come from?"

Very important point. Fixed.

> - Instead of having special logic for RPMDBI_PACKAGES in the helper,
> ?just call rpmcliArgIterHelper() with arg of NULL when it's not going
> ?to be used, this makes it plain obvious when the argv value is not
> ?used at all.

I tried this but it failed many tests.

This is because for some cases rpmgiSetArgs needs argv even if
rpmQueryVerify should have it NULL. So, for for RPMDBI_PACKAGES I had
handle it differently to keep the behaviour same.

> - The indentation of the helper is off from the general style.
> 

Fixed.

Updated: http://rakesh.fedorapeople.org/rpm/0003-Cleanup-Move-similar-patterns-in-rpmcliArgIter-to-r.patch


--
Regards,
Rakesh Pandit


[prev in list] [next in list] [prev in thread] [next in thread]