CODEWITHSUNDEEP
c++stliteratorvalgrind
abhishek_naik
abhishek_naik

Reputation: 1297

Vector iterator not being incremented

I am trying to implement topological sort using dfs (as per CLRS). I do get the required output displayed, but the program still runs and results in segmentation fault. By using few print statements for debugging, I could make out that the for loop is never exited although it should (when it==Edges.end()). However, also note that valgrind shows the error at 

dfsVisit(it->first);

within within dfs(). What am I missing? Why isn't the iterator incremented and the for loop exited? And why different reason in valgrind?

#include<cstdio>
#include<set>
#include<list>
#include<stack>
#include<algorithm>
#include<vector>
#include<utility>

struct node
{
    int d, f, value;
};

std::vector< std::pair<node, node> > Edges;
std::vector< std::pair<node, node> >::iterator it;
bool *visited;
int N, myTime=0;
node node1, node2;
void dfsVisit(node);

void dfs()
{
    for(it=Edges.begin(); it!=Edges.end(); it++)
        if(it->first.value<N)
            if(!visited[it->first.value])
                dfsVisit(it->first);
}

void dfsVisit(node n)
{
    myTime++;                           //increment myTime
    n.d=myTime;                         //set the discovery time for node n

    if(n.value<N)
        if(visited[n.value])
            return;

    for(it=Edges.begin(); it!=Edges.end(); ++it)
    {
        if(it->second.value>=N)
            continue;
        printf("In the for loop!\n");
        if(it->first.value==n.value && !visited[it->second.value])
        {
            printf("it->first.value: %d\n",it->first.value+1);
            printf("it->second.value: %d\n",it->second.value+1);
            dfsVisit(it->second);
            printf("Inside for and if\n");
        }

        printf("Inside for but outside if!\n");
        printf("Edges.end()-it: %d\n",Edges.end()-it);
    }

    visited[n.value]=true;
    myTime++;
    n.f=myTime;

    printf("For node %d, discovery time and finishing time is: %d, %d", n.value, n.d, n.f);
    return;
}

int main()
{
    int M, firstOfRule, secondOfRule, data, i;
    //node node1, node2;
    scanf("%d""%d",&N,&M);
    visited=new bool[N];

    for(i=0;i<N;i++)
        visited[i]=false;

    while(M--)
    {
        scanf("%d",&firstOfRule);
        scanf("%d",&secondOfRule);

        while(secondOfRule--)
        {
            scanf("%d",&data);
            node1.value=firstOfRule-1;
            node2.value=data-1;
            Edges.push_back(std::make_pair(node1,node2));
            printf("Pair: %d,%d\n", node1.value+1, node2.value+1);
        }
    }

    for(std::vector< std::pair<node, node> >::const_iterator it=Edges.begin(); it!=Edges.end(); ++it)
        printf("Connected %d and %d\n",it->first.value+1,it->second.value+1);

    dfs();

    return 0;
}

Output file is as below:

Pair: 1,2
Pair: 2,3
Connected 1 and 2
Connected 2 and 3
In the for loop!
it->first.value: 1
it->second.value: 2
In the for loop!
Inside for but outside if!
Edges.end()-it: 2
In the for loop!
it->first.value: 2
it->second.value: 3
In the for loop!
Inside for but outside if!
Edges.end()-it: 2
In the for loop!
Inside for but outside if!
Edges.end()-it: 1
For node 2, discovery time and finishing time is: 3, 4Inside for and if
Inside for but outside if!
Edges.end()-it: 0                         //----->  Why doesn't it exit here?
In the for loop!
Inside for but outside if!
Edges.end()-it: -1
In the for loop!
Inside for but outside if!
Edges.end()-it: -2
In the for loop!
Inside for but outside if!
Edges.end()-it: -3
... and so on until the program crashes!

Thanks for your help!

Upvotes: 2

Views: 520

Answers (1)

M.M
M.M

Reputation: 141576

You have declared std::vector< std::pair<node, node> >::iterator it; as a global variable.

But you are using it in a recursive function dfsVisit. This means that when a nested call to dfsVisit ends, it leaves it == Edges.end(). But then the earlier dfsVisit continues executing its loop, doing ++it which causes undefined behaviour.

To fix this, make it be a local variable to the dfsVisit function.

Note: If your compiler supports C++11 you can avoid some typing and use auto to declare the iterator.

Upvotes: 1

Related Questions