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

[Piotr Galiszewski]

2010/7/20 Daniel Burrows <dburrows at debian.org>:
>  Review of the package and version classes.
>
>> +      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

>> +     // name is always used during during initial sorting.
>> +     // So lazy loading doesn't have sens here.
>
>  "sense"
>
>> +     if(!candver.end())
>> +       candidate_ver = version::create(candver);
>> +     else if(pkg.CurrentVer().end() && state.Install())
>> +       candidate_ver = version::create(state.InstVerIter(*apt_cache_file));
>> +     // Return the to-be-installed version
>> +     else if(!pkg.CurrentVer().end())
>> +       candidate_ver = version::create(pkg.CurrentVer());
>> +     else
>> +     {
>> +       candidate_ver = version_ptr();
>> +       is_virtual = true;
>> +     }
>
>  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

[snip]

>
>  I don't think this is required here; load_tags already handles
> exceptions itself.
>

OK. I will remove this checks

[snip]

>
>  The reason for not caching this is presumably that the candidate
> version will handle caching?
>

Yes. I thought that the same information will be cached in to many places

>  Also, the test here is wrong: it should check whether there is a
> candidate version, not whether the package is virtual.  (same goes for
> other places that test is_virtual and then dereference
> get_candidate_ver())
>

Please see ma above comment about candidate version

>> +      const std::string &package::get_current_version() const
>> +      {
>> +     if(is_virtual)
>> +       return empty_string;
>
>  I think the actual test here should be pkg.CurrentVer().end(); this
> will be wrong for a non-virtual package with no current version.
>
>> +     if(!current_version)
>> +       current_version = pkg.CurrentVer() ? pkg.CurrentVer().VerStr() : empty_string;
>> +
>> +     return *current_version;
>> +      }
>
>  Have you clocked an actual speedup from copying the empty string?  I
> think it should be about the same as assigning from std::string().
>
>  If you're worried about unnecessary intermediates, I think that
> in_place supposedly avoids them:
>
> using boost::in_place; // I think that's the right namespace
>
>  if(pkg.CurrentVer()) // in_place() has different types in each branch
>                       // so can't use the ternary operator.
>    current_version = in_place(pkg.CurrentVer().VerStr())
>  else
>    current_version = in_place();
>
>  I would personally just as soon accept the intermediary copies in
> the interest of readability, but if you aren't sure, you could always
> time it and see if there's a difference.  My guess is that it will be
> negligible, but I've been wrong before.
>

OK. Will change that

[snip]

>> +      /** \todo This should spit text into a TextBuffer and use
>> +       *  indentation tags to align bullets (so that paragraphs wrap
>> +       *  properly).  Ideally the text of a bulleted item should align
>> +       *  with the text *following* the bullet.
>> +       */
>> +      void version::make_desc_text(const std::vector<description_element_ref> &elements,
>> +                               int level,
>> +                               std::string &out) const
>
>  Rendering logic in such a generic module raises a red flag for me.
>
>  I suggest just caching the parsed description, and letting consumers
> of this class render it.  Or, alternatively, just always re-parsing
> the description and returning a new parse each time.
>

I am little scared of performance in this case, as caching was the
main purpose for writing this classes. Displaying this description is
not a problem, because it is not frequently task. So rendering could
be done in gui. Bigger problem is searching by description. In this
case probably It will be better to give an access to unparsed
description (maybe new getter?)

[snip]

>> +     /** \brief Retrieve a reference for VerIterator object for this version. */
>
>  "Retrieve the VerIterator corresponding to this version."
>

Thanks for the review. I will fix today all issues.

>  Daniel
>
> _______________________________________________
> Aptitude-devel mailing list
> Aptitude-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/aptitude-devel
>
-- 
Regards
Piotr Galiszewski