On Mon, Oct 08, 2007 at 04:37:50PM +0200, Nagy Gabor wrote:
this is also done; but TODO: I did a dirty hack to make check_conflict visible from testdb.c (I'm not familiar in these static/symexport... stuffs, check_conflict probably should be renamed, too)
I don't know really how the interfaces for the conflict checks should be. Currently we have these 2 functions : * void check_conflict(alpm_list_t *list1, alpm_list_t *list2, alpm_list_t **baddeps, int order)
* alpm_list_t *_alpm_checkconflicts(pmdb_t *db, alpm_list_t *packages) built on top of check_conflict.
And _alpm_checkconflicts is used in sync.c and add.c . Though, after your resolve conflict patch (not merged), _alpm_checkconflicts is no longer used in sync.c . Instead, check_conflict is used directly. And now, for testdb too, check_conflict has to be used directly.
So it appears that the _alpm_checkconflicts function isn't really useful. However, the interface of check_conflict is too confusing for a public function imo. I don't know what should be done really, this requires some thinking.
I'm on the same opinion; this is an other reason for the dirty hack. But you can see that direct using of check_conflict is reasonable in many cases, so I suggest you to provide both a beginner public funtion (alpm_checkconflicts) and an advanced one (check_conflict). Or you can also choose does_conflict instead of check_conflict as an atomic conflict-compare function (analogue of alpm_depcmp), but this may lead to many duplicated codes (like alpm_depcmp ;-)
And anyway, you said these conflict checks sucked because of the assymetrical storing (I don't know how that should be handled either..).
IMHO the most cleaner would be a new pmconflict_t type, because using pmdepmissing_t confuses me often; and we can treat the two conflicting packages as sets (unordered pairs). An other possible solution: check_conflict (or add_conflict) should do the asymmetric check at least in a special order == case [before adding a (p,q) conflict pair it should check if (p,q) or (q,p) has been already added to the list.
About static/symexport stuff, first a part of the README file :
27 In a general manner, public library functions are named "alpm_<type>_<action>" 28 (examples: alpm_trans_commit(), alpm_release(), alpm_pkg_getinfo(), ...). 29 Internal (and thus private) functions should be named "_alpm_XXX" for instance 30 (examples: _alpm_needbackup(), _alpm_runscriplet(), ...). Functions defined and 31 used inside a single file should be defined as "static".
So, if I undestood correctly, you could have : 1) void SYMEXPORT alpm_conflict_check defined in conflict.c and declared in alpm.h (public) 2) void _alpm_conflict_check defined in conflict.c and declared in conflict.h (internal) 3) static void conflict_check defined in conflict.c (only used in conflict.c)
And the three could exist in the same time (1 and 2 in the same time is very common in libalpm). Thx for this useful info.
Bye, ngaba ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/