Re: [pacman-dev] [GIT] The official pacman repository branch, master, updated. v3.1.4-163-gfb09d35

Nagy Gabor wrote:

...

Nagy Gabor wrote:

...

...

commit f43805d875ad5c672afbbfff48bded2087204773 Author: Chantry Xavier Date: Sat May 10 18:47:42 2008 +0200

Cleanup usages of alpm_list_find and alpm_list_remove.

* remove obsolete and unused *_cmp helper functions like deppkg_cmp and _alpm_grp_cmp

* new alpm_list_remove_str function, used 6 times in handle.c

* remove _alpm_prov_cmp / _alpm_db_whatprovides and replace them by a more general alpm_find_pkg_satisfiers with a cleaner implementation. before: alpm_db_whatprovides(db, targ) after: alpm_find_pkg_satisfiers(alpm_db_getpkgcache(db), targ) Warning: pkg literal also satisfies pkg. But in most cases we called what_provides if we didn't find a literal.

Yes, there is no problem with this (I just emphasized it). I like the new function better because I think find_satisfiers is quite a common task. Btw, as I see, after this patch pacman -S 'kernel26>=2.6.24' automagically works, which is cool imho.

I am glad you liked at least one thing about my patch :)

...

I know, it's not exactly equivalent but I think it makes more sense that way and as you noticed, it works the same for our use case.

...

...

* remove satisfycmp and replace alpm_list_find + satisfycmp usage by _alpm_find_dep_satisfiers. before : alpm_list_find(_alpm_db_get_pkgcache(db), dep, satisfycmp) after : _alpm_find_dep_satisfiers(_alpm_db_get_pkgcache(db), dep)

Warning: possible slowdown, the old way just stopped after a satisfier (which is ideal in checkdeps), now we scan the whole db.

Right, I knew about that too, I just wanted to keep the code as clean as possible and didn't find another way. Though it might be worth to do some benchmarking / profiling. If it's really too bad, it will have to change.

One more thing, the result of alpm_find_dep_satisfiers should be freed, which is forgotten (memleak). Remember, that the old list_find was a boolean function, it was modified in order to handle causingpkg for -Ru. No doubt, this will be slower. The average slowdown of (successful) dependency check will be about 2x. (considering only the real calculations, not disk read etc.) The question is that overall this will be notable or not, this needs testing, indeed. Basically, I don't prefer clean code over "performance", so I think this should be changed (*).

I think these are valid concerns, I will send a patch which should address them, please review it :)

...

...

...

* remove _alpm_pkgname_pkg_cmp, which was used with

alpm_list_remove,

...

and use _alpm_pkg_find + alpm_list_remove with _alpm_pkg_cmp instead.

Imho this is ugly. First we find it, then we again find it via list_remove.

Yeah, it's not ideal either. But neither are dummy pkg or fake asymmetric cmp functions. I just preferred it like that. Imo the real problem here is that our data structures suck and are inefficient. Linear search ftw.

(*) is also holds here. However, performance is not an issue here, just simply we do an "unneeded" task here, which I find ugly. alpm_list_find compare functions could be easily killed, but list_remove is quite a complicated function, thus "reimplementing" won't work. I think in this case keeping pkgpkgnamecmp was better, even if it was ugly.

An alternative idea: alpm_list_find_node, which returns with an alpm_list_t node, not the ->data; and reimplement alpm_list_remove_node, but with two param list (head) and node. This information is enough to cleanly remove that node, and can be cut from the current list_remove (by splitting that function).

Since performance doesn't apply here (this code is just used by -Ru, and should not be run an insane amount of times), then I prefer clean code here. Anyway, I think we have many other more important things to worry about.