Nagy Gabor wrote: >> Nagy Gabor wrote: >>>> commit f43805d875ad5c672afbbfff48bded2087204773 >>>> Author: Chantry Xavier<[EMAIL PROTECTED]> >>>> 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. _______________________________________________ pacman-dev mailing list [email protected] http://archlinux.org/mailman/listinfo/pacman-dev
