[Aptitude-devel] (no subject)

[Manuel A. Fernandez Montecelo]

2012/2/27 Daniel Hartwig <mandyke at gmail.com>:
> -- src/pkg_grouppolicy.cc
> +                     // get an ordered list of top-sections
> +                     vector<string> topSections = aptcfg->FindVector(PACKAGE
> "::Sections::Top-Sections");
> +                     if (topSections.empty()) {
> +                             // provide some sensible defaults
> +                             topSections.push_back("main");
> +                             topSections.push_back("contrib");
> +                             topSections.push_back("non-free");
> +                             topSections.push_back("non-US");
> +                     }
> --
>
> This type of vectorized configuration really needs it's own function.
> Take a look at apt/apt-pkg/aptconfiguration.cc for some examples.
> Note how these functions cache their return values so that any
> calculations and vectorization is only performed once.

Or one can wait for a couple of months to see GCC 4.7 released and use
C++11 initialisation:

vector<string> topSections = { "main", "contrib", "non-free", "non-US" };

Which is much cleaner, and doesn't need the overhead/boilerplate of a
function to dress the trivial case.


> ** Rearranging #include lines
>
> I do not understand your motivation for rearranging these when their
> order is already from most- to least-specific, which is generally
> consistent with existing aptitude code and widely considered to be a
> good default order for c-language includes.

<explanation behind my decision>
I don't know what you're talking about.  <map> and <set> as less
specific than anything else, that's why I moved them the whole way
down; and "generic/..." (in another directory of aptitude/src) should
be less generic than the files in the same aptitude.

Wrongly or not, I reckon that aptitude is way to slow to compile, for
its size and complexity.  In previous projects, with some clever
management of header includes and forward declarations I managed to
get the compilation time down spectacularly, e.g. from 5 minutes to 1.
 Maybe I was wrong with aptitude, but that was the intention.

Starting to make commits just to fix headers seems silly to me, though.
</>

Sorry for the trouble, though.


> Whilst common amongst other programs, aptitude source files do not
> generally include two blank lines in that position.

And everybody knows that introducing spurious blank lines like that
cause great harm, and should warrant a complaint to the infidel
wreaking such havoc.


> ** Removing function comment from source file
>
> -- src/load_grouppolicy.cc
> -// Parses a chain of grouping policies from the given string and returns them.
> -pkg_grouppolicy_factory *parse_grouppolicy(string s)
> +pkg_grouppolicy_factory* parse_grouppolicy(const string& s)
> --
>
> This comment is included in the header, but why remove it from the source
> file when it is just as useful in that location?

Because it's prone to be obsoleted?

Comments about purpose of the class/function etc belong mostly in
headers (in doxygen or similar format, if possible), and
load_sortingpolicy.cc had the same comment, just not the doxygen one
(I fixed it).

Which to me, is a telling tale of a project who started without proper
documentation for years and only lately started to document things
properly, e.g. with the nagging of foreign contributors coming from
Google Summer of Code and the like.


> ** Including unrelated changes
>
> -- src/load_grouppolicy.cc
> -pkg_grouppolicy_factory *parse_grouppolicy(string s)
> +pkg_grouppolicy_factory* parse_grouppolicy(const string& s)
> --
>
> Changing from "string" to "const string &" is not related to the main
> change made in this commit, rather, it is a more general code cleanup.
> It is convenient to keep logically separate changes apart from each
> other.  This is the recommendation of the git (and other version
> control software) documentation, and is a good habit to get in to.
>
> Cleanup like this is more appropriately performed on many source files
> at once and included as a separate commit (i.e. change all functions
> where "string" can become "const string &" at the same time).  This
> way we can assess the cleanup and new functionality in
> pkg_grouppolicy_section independently.

Uhm, yes.  Such a terrible thing to fix things along the way when they
are completely harmless.  My bad.  Won't happen again.

It's better to not touch it (under penalty of a nitpicking code
review) and wait to make those changes only in The Proper Cleanup
release of aptitude (1.0), scheduled for 2030.

Cheers.