[Aptitude-devel] Review: 1ec363ac (001.1-package^)

[Piotr Galiszewski]

2010/7/20 Piotr Galiszewski <piotr at galiszewski.pl>:
> 2010/7/20 Daniel Burrows <dburrows at google.com>:
>> On Tue, Jul 20, 2010 at 9:36 AM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
>>> 2010/7/20 Daniel Burrows <dburrows at debian.org>:
>>>>> +      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