Abdelrazak Younes <[EMAIL PROTECTED]> writes:
| #ifndef IT_VECTOR_H
| #define IT_VECTOR_H
|
| #include <vector>
| #include <algorithm>
|
| #include "debug.h"
Please change order of includes. The rule is: most specific first,
most general last.
| /// vector of container iterator.
| /**
| This templatized class provide a std::vector like interface but with another
Container
| underneath. An important property is that it keeps the Container iterator
interface.
| A typical use would be:
|
| typedef it_vector<std::list<some_class> > MyContainer;
I am not sure this is the best interface...
I think I would prefere:
typedef it_vector<some_class> MyContainer;
| template <class Container>
| class it_vector
Thus:
template <class ValueType, class Container = std::list>
class it_vector
| {
| protected:
| typedef typename Container::const_iterator base_cit;
| typedef typename Container::iterator base_it;
|
| public:
|
| typedef typename Container::value_type value_type;
| typedef typename Container::difference_type difference_type;
| typedef size_t size_type;
|
| ///
| class iterator: public Container::iterator
| {
| public:
| iterator() {
| }
| iterator(base_it const & it) {
You have a coule of places with double-spaces.
| Container::iterator::operator=(it);
| }
| inline iterator & operator+= (difference_type pos) {
Please no space before the paren in a function name.
(yes, for operators as well)
And please drop the inline keyword on in-class methods, they are
implicit anyway.
| std::advance(*this, pos);
| return *this;
| }
| inline iterator operator+ (difference_type pos) const {
| iterator it=*this;
| std::advance(it, pos);
| return it;
| }
| inline difference_type operator- (iterator it2) const {
| return std::distance(*this, it2);
| }
I agree with the criticism against these functions. They should both
do the same thing: move iterators forward and backwards.
| inline const iterator & operator= (base_it const & it) {
| Container::const_iterator::operator=(it);
| return *this;
| }
This looks wrong... a bit...
I think the iterator class should be built from ground up, and not
inherit from the Container::iterator. With several operations you miss
the faster iterator operations a the vector can give.
(You have the vector, and can thus get O(1) operator+, but not if you
use the list::iterator.)
| ///
| it_vector() {
Way too many spaces.
| }
|
| /// Construct a new it_vector by copying externalItVector.
You don't really have to comment this :-)
We do know what a copy constructor is.
| it_vector(it_vector const & externalItVector) {
| for (const_iterator it = externalItVector.begin(); it !=
externalItVector.end(); ++it)
| push_back(*it);
| }
| /// Construct a new it_vector by copying externalItVector from start to
end.
| it_vector(it_vector const & externalItVector, size_t start, size_t end)
{
| for (size_t i = start; i != end; ++i)
| push_back(externalItVector[i]);
| }
(pos, length)
| /// Construct a new it_vector by copying elements pointed from it_start
to it_end.
| it_vector(const_iterator it_start, const_iterator it_end) {
| for (const_iterator it = it_start; it != it_end; ++it)
| push_back(*it);
| }
Template.
|
| /// Copy externalItVector.
| inline void copy(it_vector const & externalItVector) {
| copy(externalItVector, 0, externalItVector.size());
| }
|
| /// Copy externalItVector from it_start to it_end.
| void copy(const_iterator it_start, const_iterator it_end) {
| clear();
| for (const_iterator it = it_start; it != it_end; ++it)
| push_back(*it);
| }
Copy is the wrong name for this. In the other containers this is
called 'assign'.
And if you make the ones taking iterators a template instead, you only
need one to cover both const and non-const iterators.
(and will be able to assign form other containes than it_vector)
| /// Copy externalItVector from start to end.
| void copy(it_vector const & externalItVector, size_t start,
| size_t end) {
Other containers use "pos, length" don't they?
| inline value_type & at(size_t pos) {
| if (pos >= ItVector_.size()) {
| lyxerr[Debug::ACTION] << "trying to access element " <<
pos << " while size is "<< size() << endl;
| return *ItVector_[size()-1];
| }
| BOOST_ASSERT(pos < ItVector_.size());
| return *ItVector_[pos];
| }
|
| inline value_type const & at(size_t pos) const {
| if (pos >= ItVector_.size()) {
| lyxerr[Debug::ACTION] << "trying to access element " <<
pos << " while size is "<< size() << endl;
| return *ItVector_[size()-1];
| }
| BOOST_ASSERT(pos < ItVector_.size());
| return *ItVector_[pos];
| }
Do we really need these checked functions? Won't the gcc debug lib do
this for us? (and where is operator[]?)
| /// Erases the paragraphs from start to end.
| /**
| \todo This could be optimized...
| */
| bool erase(size_t start, size_t end) {
| bool result=true;
| for (size_t i = start; i != end; ++i)
| result = result && erase(i);
|
| return result;
| }
Hmm... (pos, length)?
Ok, got tired.
Will do more later.
I think you should think of ItVector as just and implemention detail,
and keep it away from naming. Also and invalidation and rebuilding
from ground up of ItVector, but in some cases be a better option that
having the two containers walk in step.
F.ex. the operation of inserting a list into the container is now a
2 x O(n) operation. At least with list and splice it should be
possible to have this as O(1) + O(n).
I also think that it_vector really should be a RandomAccessList, tuned
for only working with std::list as the main container and std::vector
as its 'iterator cache'
--
Lgb
