On 27 February 2012 18:45, Manuel A. Fernandez Montecelo
manuel.montez...@gmail.com wrote:
2012/2/27 Daniel Hartwig mand...@gmail.com:
-- src/pkg_grouppolicy.cc
+ // get an ordered list of top-sections
+ vectorstring 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:
vectorstring 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
