views:

105

answers:

3

I'm working on a program that's supposed to represent a graph. My issue is in my printAdjacencyList function. Basically, I have a Graph ADT that has a member variable "nodes", which is a map of the nodes of that graph. Each Node has a set of Edge* to the edges it is connected to. I'm trying to iterate across each node in the graph and each edge of a node.

void MyGraph::printAdjacencyList() {
std::map<std::string, MyNode*>::iterator mit;
std::set<MyEdge*>::iterator sit;

for (mit = nodes.begin(); mit != nodes.end(); mit++ ) {
    std::cout << mit->first << ": {";
    const std::set<MyEdge*> edges = mit->second->getEdges();
    for (sit = edges.begin(); sit != edges.end(); sit++) {
        std::pair<MyNode*, MyNode*> edgeNodes = *sit->getEndpoints();
    }
}
std::cout << " }" << std::endl;
}

getEdges is declared as:

const std::set<MyEdge*>& getEdges() { return edges; };

and get Endpoints is declared as:

const std::pair<MyNode*, MyNode*>& getEndpoints() { return nodes; };

The compiler error I'm getting is:

MyGraph.cpp:63: error: request for member `getEndpoints' in 
`*(&sit)->std::_Rb_tree_const_iterator<_Tp>::operator->
[with _Tp = MyEdge*]()', which is of non-class type `MyEdge* const'

MyGraph.cpp:63: warning: unused variable 'edgeNodes'

I have figured out that this probably means I'm misusing const somewhere, but I can't figure out where for the life of me. Any information would be appreciated. Thanks!

+4  A: 

Try changing sit to a const_iterator. Change mit to a const_iterator too while you're at it. Also, getEdges() and getEndpoints() should be const functions. Lastly, because operator->() has a higher precedence than the unary operator*(), you probably want to say edgeNodes = (*sit)->getEndPoints() inside the inner-loop.

Not as much of a problem but you should consider having the iterator instances as local to the loops.

wilhelmtell
I've changed both iterators to const_iterators and redefined getEdges and getEndpoints to const functions, no change in compiler error.
Jared
@Jared I updated the answer. I think these changes should calm down the compiler.
wilhelmtell
If you still get these errors after these, what happens if you change the return types of those accessors to a return-by-value, instead of return by const-reference?
wilhelmtell
That last comment was before the (*sit)->getEndpoints() suggestion. That solved my problem right up, thanks!
Jared
+1  A: 

s/std::set<MyEdge*>::iterator sit;/std::set<MyEdge*>::const_iterator sit;/

and ditto for mit. In other words, const_iterator is what you need here.

bmargulies
I've changed both iterators to const_iterator and the error is exactly the same.
Jared
+2  A: 

It's hard to tell anything definite without something compilable, but

const std::set<MyEdge*>& getEdges() { return edges; };

and

const std::pair<MyNode*, MyNode*>& getEndpoints() { return nodes; };

should technically be const methods since they don't modify the class and return a const reference.

const std::set<MyEdge*>& getEdges() const { return edges; };
const std::pair<MyNode*, MyNode*>& getEndpoints() const { return nodes; };

This in combination with const_iterator might solve your constness problems.


However, your particular error might be that *it->foo() = *(it->foo())is different from (*it)->foo()

UncleBens
(*it)->foo() did the trick! What exactly do the parens do? Just tell the compiler to treat the dereferenced iterator as an object?
Jared
They change the precedence. The -> operator has more precedence than the * operator, so it is evaluated first. But you need the * to be evaluated first, so you enclose it with the operand inside the parenthesis.
Fabio Ceconello