[Aptitude-devel] (no subject)

[Daniel Hartwig]

On 27 February 2012 18:45, Manuel A. Fernandez Montecelo
<manuel.montezelo at gmail.com> wrote:
> 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.
>

Such a function permits:

  - memoization of the vector;
  - error checking on the values (if useful/relevant);
  - consistent defaults from many places in the code (i.e. code reuse).

Any way, I still think this code is better to at least be placed in
the constructor.  As mentioned, it is needed to also fix this latent
bug which assumes "main" instead of using the first element of
Top-Sections:

      if(split_mode == pkg_grouppolicy_section_factory::split_topdir)
	section = (first_split != section.npos
		   ? section.substr(0,first_split)
		   : _("main"));


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

Not at all.  The comment about rearranging was more directed at the
changes to load_grouppolicy.cc rather than pkg_grouppolicy.cc.

Regardless, I was not aware that such changes would have a noticable
effect on compilation times -- so there you go :-)


>
>> ** 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?
>

True.


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

Ok, so this was meant to be a small note about the utility of
logically distinct commits -- which is generally considered good
practice.  I am not aware if you were aware of this, so I have
attempted to politely point it out since it seemed relevant.  Surely
it is such a minor issue that would not have been mentioned had I not
already been asking about other aspects.

Perhaps read it more as an open invitation to "please consider making
a larger commit which addresses the const-correctness of many
functions."