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

[Daniel Burrows]

  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.

> +	// 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).

> +      package::tag_iterator package::tags_begin() const
> +      {
> +	if(is_virtual && !tags)
> +	  tags = std::set<tag>();
> +
> +	if(!tags)
> +	  tags = aptitude::apt::get_tags(pkg);
> +
> +	return (*tags).begin();
> +      }

  I'd prefer using a helper routine and cleaning up the logic:

  void package::ensure_tags() const
  {
    if(!tags)
      {
        if(is_virtual)
          tags = std::set<tag>();
        else
          tags = aptitude::apt::get_tags(pkg);
      }
  }

  package::tag_iterator package::tags_begin() const
  {
    ensure_tags();
    return (*tags).begin();
  }

  package::tag_iterator package::tags_end() const
  {
    ensure_tags();
    return (*tags).begin();
  }

  Also, I wonder if you really need to check for a virtual package
here -- I thought get_tags() was robust against that.

  Note: usually where you see if(!x) x = a; if(!x) x = b; the reason
is for robustness, because the first assignment might fail for some
reason.  I don't see that as being likely here.  Now what might be
better is something like:

  if(!tags)
    {
      try
        {
          tags = load_tags(pkg);
        }
      catch(...) // Should catch std::exception and cw::util::Exception and log.
        {
          tags = std::set<tag>();
        }
    }

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

> +      void package::load_versions() const
> +      {
> +	versions = std::vector<version_ptr>();
> +
> +	if(!is_virtual)

  Not necessary -- virtual packages by definition have no versions.

> +	  for(pkgCache::VerIterator i=pkg.VersionList(); !i.end(); i++)
> +	    if(i!=get_candidate_ver().get()->get_ver())
> +	      (*versions).push_back(version::create(i));
> +	    else
> +	      (*versions).push_back(get_candidate_ver());

  This is OK for now.  Long run, I think versions (and packages)
should be flyweights, which would make this check unnecessary.

> +      const std::string &package::get_archive() const
> +      {
> +	return is_virtual?empty_string:get_candidate_ver().get()->get_archive();
> +      }

  Could you break this line up a little more?

    return is_virtual
             ? empty_string
             : get_candidate_ver()->get_archive();

  Same for get_candidate_version().

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

  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())

> +      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.


  If you do go with your current code, please break the long ?: line
up as illustrated earlier.

> +      const std::string &package::get_short_description() const
> +      {
> +	return is_virtual?short_desc_fallback:get_candidate_ver().get()->get_short_description();

  As noted earlier, it would be preferable to initialize
short_desc_fallback here.

  Also, in this and other places where you use the .get()-> idiom, I'd
suggest just relying on the operator-> overload of the shared_ptr
class:

  return is_virtual
           ? short_desc_fallback // TODO: initialize this here
           : get_candidate_ver()->get_short_description();

> +namespace aptitude
> +{
> +  namespace gui
> +  {
> +    namespace qt
> +    {
> +      class package;
> +      typedef boost::shared_ptr<package> package_ptr;
> +
> +      /** \brief Package is an object representing each displayable packages.

  "An object representing a displayable package."

> +       *  It is also possible to retrieve raw PkgIterator object for this package

  "the raw PkgIterator object for this package."

> +	/** \brief Retrieve archive, which contains candidate version of this package. */

  "Retrieve the archive which contains the candidate version of this package."

> +	/** \brief Retrieve candidate version for this package. */

  "Retrieve the candidate version number of this package."

> +	/** \brief Retrieve installed version of this package. */

  "Retrieve the installed version number of this package."

> +	const std::string &get_current_version() const;

> +	/** \brief Retrieve project's homepage. */

  "Retrieve the package's homepage."

> +	/** \brief Retrieve long description of this package. */

  "Retrieve the long description of this package."

> +	/** \brief Retrieve mainatiner of this package. */

  "Retrieve the maintainer of this package." (note spelling)

> +	/** \brief Retrieve name of this package. */

  "Retrieve the name of this package."

> +	/** \brief Retrieve priority of this package. */

  +the

> +	/** \brief Retrieve section this package belongs. */

  "Retrieve the section to which this package belongs."

> +	/** \brief Retrieve short description of this package. */

  "Retrieve the short description of this package."

> +	/** \brief Retrieve name of source package for this package. */

  "Retrieve the name of this package's source package."

> +	/** \brief Retrieve a reference for PkgIterator object for this package. */

  "Retrieve the PkgIterator corresponding to this package."

> +	/** \brief Retrieve a reference to version_ptr object for candidate version
> +	   *  of this package.
> +	   */

  "Retrieve the candidate version of this package, or an invalid reference
   if the package has no candidate version."

> +      /** \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.

> +      /** \brief version is an object representing each displayable versions of package.

  "An object representing a displayable version of a package."

> +       *  It is also possible to retrieve raw VerIterator object for this version

  "It is also possible to retrieve the raw VerIterator object for this version."

> +	/** \brief Retrieve archive, which contains candidate version of this version. */

  "Retrieve a description of the archives that contain this version."

  (long-term we should probably return the archives as some sort of
   collection?)

> +	/** \brief Retrieve project's homepage. */

  "Retrieve the package's homepage."

> +	/** \brief Retrieve long description of this version. */

  "Retrieve the long description of this version."

> +	/** \brief Retrieve mainatiner of this version. */

  +the

> +	/** \brief Retrieve priority of this version. */

  +the

> +	/** \brief Retrieve short description of this version. */

  +the

> +	/** \brief Retrieve name of source package for this version. */

  "Retrieve the name of this version's source package."

> +	/** \brief Retrieve string representing this version. */

  "Retrieve the version number of this version."

> +	const std::string &get_ver_string() const;

  Suggest instead get_version_number(); and corresponding changes to
other names.

> +	/** \brief Retrieve a reference for VerIterator object for this version. */

  "Retrieve the VerIterator corresponding to this version."

  Daniel