2010/7/20 Piotr Galiszewski <[email protected]>: > 2010/7/20 Daniel Burrows <[email protected]>: >> On Tue, Jul 20, 2010 at 9:36 AM, Piotr Galiszewski <[email protected]> >> wrote: >>> 2010/7/20 Daniel Burrows <[email protected]>: >>>>> + package::package(const pkgCache::PkgIterator &_pkg) >>>>> + : pkg(_pkg), is_virtual(false), short_desc_fallback(_("virtual")) >>>> >>>> I wonder if it would be better to default-construct >>>> short_desc_fallback (i.e., don't list it) and then assign it from >>>> _("virtual") later, to avoid looking up _("virtual") for non-virtual >>>> packages. Or if you really want to avoid the default-construction, >>>> use a ternary operator to avoid it. >>>> >>> >>> I tried to make this variable const. I will fix it and initialize this >>> variable later >> >> I don't think there's a big payoff for constness here, since the >> whole class is based on using mutable members to cache. >> > > OK > >>>> Is that logic correct? I think you should only set is_virtual if >>>> there are no versions at all (i.e., pkg.VersionList().end() is true). >>>> >>> >>> True. I had a problem with this. How should things work for such >>> packages. Searching and filtering for packages uses candidate version >>> now (through package getters). I am not quite sure if it is good >>> thing. Maybe I should add new method: get_version and use it instead >>> of get_candidate_version. get_version will return candidate version if >>> available, otherwise the newest version of package. All checks have to >>> be fixed in this situation >> >> At the least, you shouldn't use is_virtual to mean something >> different from the rest of the codebase; that will lead to confusion. >> >> Other frontends have the concept of the "visible version" of >> a package; I'd suggest using that. is_virtual will then be true iff >> there is no visible version -- although I might just check whether >> the visible version is NULL instead. >> > > Ah. I see (looking at pkg_item). I mixed logic of candidate version > and visible version. It looks like I should rename candidate_version > to visible_version and use it as previous. Furthermore, pkt_item's > visible_version uses the the same code as my candidate_version. I have > only one more questions. If I read it correctly this visible_version > returns NULL in the same situations as my candidate_version. > > In the second version of my patch (the first version without Qt > dependencies), I used proper logic for is_virtual and > candidate_version (should have been visible_version). I have changed > that code after first real testing of this code. My code crashes > frequently when is_virtual was false and candidate_version returned > null. I based that code on pkg_tree and pkg_item, but probably I > missed some important checks > > Should visible_version returns something in every situation > pkg.VersionList().end() returns true? >
I pushed new version. Maybe now it will be better ;) -- Regards Piotr Galiszewski _______________________________________________ Aptitude-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
