Std Iterator Problem

[Adam Hartshorne]

Hi All,

I was wondering if anybody can tell me what is wrong with the following
code,

in a .h file I have

std::list<std::vector<Site> > positions ;
std::list<std::vector<Site> >::iterator itSteps ;

then in the .cpp file

void RmfSystem::step() {

if (itSteps == positions.end()) {

itSteps = positions.begin() ;

} else {

itSteps++ ;

}

}

where step gets called by an animate function in another file. The idea
that I have a vector of vector of positions and I want on each call of
step that the iterator points at another set of positions. Then when the
end of the list (well vector) is reached it loops around to the start.

As it stands my program crashes, as itStep == positions.end() never
seems to be found to be true.

Any ideas what I am doing wrong?

Adam

 

[Larry I Smith]

Hi All,

I was wondering if anybody can tell me what is wrong with the following
code,

in a .h file I have

std::list<std::vector<Site> > positions ;
std::list<std::vector<Site> >::iterator itSteps ;

then in the .cpp file

void RmfSystem::step() {

if (itSteps == positions.end()) {

itSteps = positions.begin() ;

} else {

itSteps++ ;

}

}

where step gets called by an animate function in another file. The idea
that I have a vector of vector of positions and I want on each call of
step that the iterator points at another set of positions. Then when the
end of the list (well vector) is reached it loops around to the start.

As it stands my program crashes, as itStep == positions.end() never
seems to be found to be true.

Any ideas what I am doing wrong?

Adam

Among other things, where is the code that initializes
'itSteps'? Somewhere (after 'positions' is filled with
data) you need to do:

itSteps = positions.begin();

A global iterator can cause unexpected side effects
if multiple functions modify the container that
the iterator references. This is a risky design.

Regards,
Larry

 

[Peter Kragh]

in a .h file I have

std::list<std::vector<Site> > positions ;
std::list<std::vector<Site> >::iterator itSteps ;

then in the .cpp file

void RmfSystem::step() {

if (itSteps == positions.end()) {

itSteps = positions.begin() ;

} else {

itSteps++ ;

}

}

I don't see any errors in the above code per se. However, it's generally
*not* a good idea to define global variables in a header file.

My guess is that the error can be found elsewhere, e.g. calling erase.
It /can/ invalidate your iterator.

 

[Adam Hartshorne]

Among other things, where is the code that initializes
'itSteps'? Somewhere (after 'positions' is filled with
data) you need to do:

itSteps = positions.begin();

A global iterator can cause unexpected side effects
if multiple functions modify the container that
the iterator references. This is a risky design.

Regards,
Larry

Sorry yes I have the following method

void RmfSystem::setUpAnimation() {

itSteps = positions.begin() ;

}

and it works fine, but like I say fails to find match the .end and hence
then loop around.

How should I do this instead?

Adam

 

[Adam Hartshorne]

Sorry yes I have the following method

void RmfSystem::setUpAnimation() {

itSteps = positions.begin() ;

}

and it works fine, but like I say fails to find match the .end and hence
then loop around.

How should I do this instead?

Adam

I think I have worked out the problem, but not sure how to solve it. I
create the iterator at the same time as the list is created. However, I
then go on to fill the list up. I need to really create/renew the
iterator when the list has been finished been filled, but not sure how I
can do that, and still have it globally available.

Adam

 

[Andre Kostur]

I think I have worked out the problem, but not sure how to solve it. I
create the iterator at the same time as the list is created. However,
I then go on to fill the list up. I need to really create/renew the
iterator when the list has been finished been filled, but not sure how
I can do that, and still have it globally available.

Since you're playing with vector<>, you could use an index instead of an
iterator. Just be careful that your vector doesn't get _shorter_ while
you hold an index into the vector. Also watch out for iterator validity
rules. Remember that on insert, all iterators and references into the
vector may be invalidated. So... doing:

std::vector<int> vec;

// assume vec has say 5 items in it already

int index = 2;

int & vecvalue = vec[index];

vec.push_back(6);

vecvalue = 7;


Is "illegal". After the push_back, the reference vecvalue is no longer
valid. However, if you:

std::vector<int> vec;

// assume vec has say 5 items in it already

int index = 2;

int vecvalue = vec[index];

vec.push_back(6);

vec[index] = 7;

Is "correct". After the push_back, we re-index back into the vector, so
it recalculates on the potentially new area of memory that the vector now
occupies...

So, your step() method becomes:

void RmfSystem::step()
{
if (itSteps < positions.size())
{
++itSteps;
}
else
{
itSteps = 0;
}
}

 

[Howard]

I think I have worked out the problem, but not sure how to solve it. I
create the iterator at the same time as the list is created. However, I
then go on to fill the list up. I need to really create/renew the iterator
when the list has been finished been filled, but not sure how I can do
that, and still have it globally available.

Adam

Just make sure to set the iterator to begin() *after* filling up the list.
Also, reset it if you remove anything from the list. But why not make those
both members of the RmfSystem class? You can then use member functions of
that class to get or set any Site items using the iterator. (At least move
them out of the header, anyway.)

-Howard

 

[Adam Hartshorne]

Just make sure to set the iterator to begin() *after* filling up the list.
Also, reset it if you remove anything from the list. But why not make those
both members of the RmfSystem class? You can then use member functions of
that class to get or set any Site items using the iterator. (At least move
them out of the header, anyway.)

-Howard

I have just checked, and I do set set the iterator to begin() after
filling up the list.

Also both are memebers of the RmfSystem Class in the header definition.
To achieve what you suggest, I'm not sure how to do that. Also how can
you 'reset' the iterator?

Adam

 

[Larry I Smith]

I think I have worked out the problem, but not sure how to solve it. I
create the iterator at the same time as the list is created. However, I
then go on to fill the list up. I need to really create/renew the
iterator when the list has been finished been filled, but not sure how I
can do that, and still have it globally available.

Adam

After the container is filled do:

itSteps = positions.begin();

Perhaps you can just call your RmfSystem::setUpAnimation()
method after the container is filled.

The way you're using these globals is very risky.
It is not normally done, for good reasons. Creating
them in a header file (that may be included by multiple
source files) is a bad idea; you may end up with multiple
copies of them in multiple object files. To ensure that
you get just one copy of each global, try this:

in Site.h:

// state that the global 'positions' and 'itSteps' exist
// somewhere
extern std::list<std::vector<Site> > positions ;
extern std::list<std::vector<Site> >::iterator itSteps ;

in Site.cpp:

// create the global 'positions' and 'itSteps'
std::list<std::vector<Site> > positions ;
std::list<std::vector<Site> >::iterator itSteps ;

Any changes to the container by any function may
invalidate the iterator.

I have no idea what you're trying to do, but perhaps
using a class to encapsulate the container , access to it,
and changes to it, would be a better approach.

Start with a good book on OO Design...

Regards,
Larry

 

[Howard]

You haven't shown the context in which this is used, but I think I see the
problem. If I recall correctly, the iterator end() points to one past the
last valid member of the list, *not* to the last member itself. Given that,
the function should be:

++itSteps;
if (itSteps == positions.end())
itSteps = positions.begin();


(I suppose you could pre-increment inside the if statement, but I prefer to
limit actions to one per line, personally.)

-Howard

 

[Peter Koch Larsen]

Hi All,

I was wondering if anybody can tell me what is wrong with the following
code,

in a .h file I have

std::list<std::vector<Site> > positions ;
std::list<std::vector<Site> >::iterator itSteps ;

then in the .cpp file

void RmfSystem::step() {

if (itSteps == positions.end()) {

itSteps = positions.begin() ;

} else {

itSteps++ ;

}

*** 2

}

where step gets called by an animate function in another file. The idea
that I have a vector of vector of positions and I want on each call of
step that the iterator points at another set of positions. Then when the
end of the list (well vector) is reached it loops around to the start.

As it stands my program crashes, as itStep == positions.end() never seems
to be found to be true.

Any ideas what I am doing wrong?

Adam

Apart from what others say, I must ask you where you "use" your iterator.
You are not allowed to use it in *** 2 indicated above as you might
dereference end().

/Peter

 

[Adam Hartshorne]

Adam Hartshorne wrote:



I think I have worked out the problem, but not sure how to solve it. I
create the iterator at the same time as the list is created. However,
I then go on to fill the list up. I need to really create/renew the
iterator when the list has been finished been filled, but not sure how
I can do that, and still have it globally available.

Since you're playing with vector<>, you could use an index instead of an
iterator. Just be careful that your vector doesn't get _shorter_ while
you hold an index into the vector. Also watch out for iterator validity
rules. Remember that on insert, all iterators and references into the
vector may be invalidated. So... doing:

std::vector<int> vec;

// assume vec has say 5 items in it already

int index = 2;

int & vecvalue = vec[index];

vec.push_back(6);

vecvalue = 7;


Is "illegal". After the push_back, the reference vecvalue is no longer
valid. However, if you:

std::vector<int> vec;

// assume vec has say 5 items in it already

int index = 2;

int vecvalue = vec[index];

vec.push_back(6);

vec[index] = 7;

Is "correct". After the push_back, we re-index back into the vector, so
it recalculates on the potentially new area of memory that the vector now
occupies...

So, your step() method becomes:

void RmfSystem::step()
{
if (itSteps < positions.size())
{
++itSteps;
}
else
{
itSteps = 0;
}
}

No using a vector, it is a list of vectors. I am iterating through the
list. I have appeared to solve my problem, but replacing

if (itSteps == positions.end()) {

with

if (itSteps == --positions.end()) {

appears to work correctly.

 

[Andre Kostur]

No using a vector, it is a list of vectors. I am iterating through the
list. I have appeared to solve my problem, but replacing

if (itSteps == positions.end()) {

with

if (itSteps == --positions.end()) {

appears to work correctly.

Undefined behaviour if positions is empty.

 

[Peter Koch Larsen]

Andre Kostur wrote:
[snip]

if (itSteps == positions.end()) {

with

if (itSteps == --positions.end()) {

appears to work correctly.

As Andre pointed out, this is undefined if the list is empty. It is also an
indication that you are using an invalid iterator as i explained in my other
reply.

/Peter