Whoua... Another set of comments!
Lars Gullik Bjønnes a écrit :
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.
OK
| /// 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;
I agree and I actually did that in my first version but it didn't want
to compile so I opted for this simpler approach.
| template <class Container>
| class it_vector
Thus:
template <class ValueType, class Container = std::list>
class it_vector
OK, if we manage to fix the compile and it doesn't complicate the code
too much.
| {
| 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.
I'll fix that.
| 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.
I thinks this depends on the compiler but OK. I got used to do that
because of crappy VC++ 6.
| 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.
I don't understand. These two operators have nothing to do one with the
other. "operator+" takes a difference_type argument and "operator-"
takes an iterator. I think you are mis leaded by the fact operator- is
defined inside the class instead of:
difference_type operator-(iterator it1, iterator it2) const {
return std::distance(it1, it2);
}
| 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.)
IMHO, you want the butter and the money of the butter. For me it is
important that iterator _is_ a list::iterator so that it is compatible
with an external list::iterator independent from it_vector. If you want
faster access, use the operator[] instead. That's what I meant when I
said "fix the ParagraphList usage to use O(1) interface.
| ///
| it_vector() {
Way too many spaces.
OK.
| }
| | /// Construct a new it_vector by copying externalItVector.
You don't really have to comment this :-)
We do know what a copy constructor is.
I hope you do ;-). This is just the result of copy and paste. And I was
told way too often that I don't comment enough.
| 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)
OK
| /// 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.
OK
| | /// 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'.
'copy' means more to me that 'assign' but OK.
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)
I'll try this, thanks.
| /// 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?
Yes but I don't aim for BOOST insertion :-). I never really liked to
always have to mentally calculate the difference whereas I have two
positions. That said I agree that it could be misleading for STL and
BOOST users.
| 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[]?)
I know that STLport does but mingw STL does not.
| /// 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)?
See above.
Ok, got tired.
Will do more later.
Good job anyway :-)
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.
I have tried that and it doesn't make a difference on my system even
with the largest document I have. This is because the ItVector
insertion is cheap (value_type is a simple iterator) and IMHO cleaner
than a complete rebuilding.
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 am open to suggestion for fixing that. But I would like to say that it
is way better than std::vector. Actually, insertion of multiple
paragraph with graphics etc is almost instantaneous.
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'
I need more explanation.
