> On Fri, Jul 29, 2011 at 12:40:12AM +0200, Michel Hermier
> <[email protected]> wrote:
>> I need some people (vmiklos) to review the preliminary attached changes.
>> (Note they are not yet ment to be compiled as this).
>
> So I'm just commenting your ideas, I can't really review a patch that
> does not compile / not split it up to smaller, reviewable patches.
>
>> In there you can see 3 axes of changes.
>> - add.* remove.* sync.* trans.*: The first one that is functionnal and
>> need still a bit more love (but that is currently enought for compiling)
>> is a kind of transaction engine controled by structs. Instead of using a
>> switch for each transaction, we use function from the struct. It helps
>> to
>> reduce the number of private exported symbols and make the libpacman a
>> little bit more kernel style.
>
> private exported symbols? I you want to reduce that, then just add
> visibility markup and all will be gone. If you want to instruce such
> structs with function pointers, I would rather refactor the db code:
> that is really full of if (local) .. else <assume remote> code chunks,
> and there such refactoring would *do* make the code more readable.
I didn't reached that code but yes. Currently the aims is at least to try
to make the logic a little bit more readable and more object style.
(If I can try to split the upgrade code out of remove and reduce some code
duplication)
> Note that in the db access case there are two users of that structs,
> refactoring to use such a struct with a single user is unnecessary
> complication, IMHO.
It add a little complication, but can help if tomorrow we need another db
provider (like transparant db over network or whatever)
>> - package.*: The second changes is a proposal to change of the package
>> descrition that allows the definition of RODEPEND package variables to
>> be
>> exported. This extension as an intimate relation with the rodepend
>> FrugalBuild but can be used to do a little bit more. This would help us
>> to
>> reduce the hidden dependencies introduced by the package plugin
>> dependencies, using a new tricky variable I have to name. (This part is
>> not functionnal, the current part only expose the description loader
>> change required to load the modified files).
>
> As I said on IRC, I don't think this makes too much sense (it just makes
> building a little bit faster, zero benefit to end-users), but sure, as
> long as you are willing to do the work, and fix packages in case
> somewhere we depend on the fact that rodepends are currently installed
> in the build chroot, why not. ;)
Well for now I don't know how to concontinue that. The code looks so
complicated that I didn't find where packages from descriptors gets
accepted for installation.
>> - utils.*: Introduce _pacman_{free,malloc,zalloc} functions as generic
>> memory helpers. the malloc one would reduce the code duplication for
>> error
>> handling, while the others are just syntax sugars to perform some usual
>> memory stuff. (Yes I know we have the FREE macro, but it is usually not
>> safe because we tends to forget that arguments are recomputed at each
>> usage. Making the code hard to debug sometimes)
>
> That's why C99 or somesuch introduced inline functions. But in case FREE
> is about free(var); var = NULL, all an inline function can do is
> inline_free(&ptr), and cluttering the code with a lot of "pointer to a
> pointer" sounds ugly..
I agree this free is overkill, I can remove that. It was meant for
providing something. Else maybe I can make it a functionnal macro (so that
it follows the _pacman_malloc new pattern), but that would make a copy of
FREE.
_______________________________________________
Frugalware-devel mailing list
[email protected]
http://frugalware.org/mailman/listinfo/frugalware-devel
