overriding == operator in order to test equality of two pointers

[lucky6969b]

The new np will have a different address, if I get the actualCosts which is a std::map,
I can't seem to find the correct pair from the container.
I try to override the == operator to make sure they compare correctly, and then failed.
Any ideas I can get this to work?
Thanks
Jack

 
float Grid::getActualCost(int fromX, int fromZ, int toX, int toZ) {		 	 
	 
	AStarNode* from = grid[fromX][fromZ];
	AStarNode* to = grid[toX][toZ];
	AStarNodePair* np = new AStarNodePair(from, to);
	float cost = actualCosts.at(np);	 
	return cost; 
}

#ifndef ASTARNODEPAIR_H_
#define ASTARNODEPAIR_H_

#include "AStarNode.h"

class AStarNodePair {
	AStarNode* from;
	AStarNode* to;

public:
	AStarNodePair(AStarNode* f, AStarNode* t)  {
		this->from = f;
		this->to = t;
	}

	bool operator < (AStarNodePair* other) {
		if ((from->id == other->from->id) && (to->id == other->to->id)) {
			return false;
		}
		if ((from->id == other->from->id) && (to->id < other->to->id)) {
			return true;
		}
		if ((from->id == other->from->id) && (to->id >= other->to->id)) {
			return false;
		}
		if (from->id < other->from->id) {
			return true;
		}
		if (from->id >= other->from->id) {
			return false;
		}


	}

	bool operator == (AStarNodePair* other) {
		if ((from->GetX() == other->from->GetX()) && (to->GetZ() == other->to->GetZ())) {
			return true;
		}
		return false;

	}


};

#endif // ASTARNODEPAIR_H_

[laserlight]

The new np will have a different address, if I get the actualCosts which is a std::map,
I can't seem to find the correct pair from the container.

What is the exact type of the std::map?

I try to override the == operator to make sure they compare correctly, and then failed.

You meant overload, not override. The term override is used specifically when you want to override a virtual function. Anyway, std::map uses operator< by default, so overloading operator== makes no difference to it, though generally if you overload operator< people would expect you to also overload operator==.

The issue is that your overload of operator< is incorrect: you should overload it such that (x < y) is valid for const objects x and y that are of the key type of the std::map. If you want it to be a member function of AStarNodePair, then the member function should be declared const, and the parameter should be a const reference to AStarNodePair. If not, it should be a non-member function with two parameters.

[lucky6969b]

Hi laserlight,
I put a breakpoint within this method, but the prog never pauses at that breakpoint.
Any ideas why?

The from Node and to Node are both const within the class now.

std::map<AStarNodePair*, float> actualCosts;

bool AStarNodePair::operator < (const AStarNodePair* other) const {
 	if ((from->GetX() == other->from->GetX()) &&
		(from->GetZ() == other->from->GetZ()) &&
		(to->GetX() == other->to->GetX()) &&
		(to->GetZ() == other->to->GetZ())) {
		return false;
	}
	else if
		((from->GetX() == other->from->GetX()) &&
		(from->GetZ() < other->from->GetZ()) &&
		(to->GetX() < other->to->GetX()) &&
		(to->GetZ() < other->to->GetZ())) {		 
		return true;
	}
	else if	
		((from->GetX() == other->from->GetX()) &&
		(from->GetZ() == other->from->GetZ()) &&
		(to->GetX() < other->to->GetX()) &&
		(to->GetZ() < other->to->GetZ())) {
		return true;
	}
	else if	
		((from->GetX() == other->from->GetX()) &&
		(from->GetZ() == other->from->GetZ()) &&
		(to->GetX() == other->to->GetX()) &&
		(to->GetZ() < other->to->GetZ())) {

		return true;
	}
	return false;
}

[laserlight]

I put a breakpoint within this method, but the prog never pauses at that breakpoint.
Any ideas why?

Because you overloaded operator< such that a AStarNodePair object is expected on the left hand side but a AStarNodePair pointer is expected on the right hand side. This is consequence of overloading it as a member function.

Now, pointers can already be compared with operator< so you should define a named function or function object to serve as the comparator to compare two AStarNodePair pointers. This function or function object should then be provided to the std::map.

[OReubens]

you have a map with AStarNodePair* as key, so it compares pointers, not object instances. You can't overload member comparison with a pointer, because there is never a AstarNodePair-pointer-class that can serve as the 'left' part of the comparison operator.

so if you insist on a map with pointers as key, you need a global operator that compares 2 AStarNodePair pointers, ideally though not necessarily this should be comparing const pointers.

Also tip, if you are writing a comparison operator that works on a series of independant segments (such as here where you sort by 'from' first, then 'to').
you can make this very easily by using the tupple comparison operator, and tying your values into a tupple "automagically", this is particularly nice if you have multiple segments as the comparison cascade can be quite elaborate when done by hand.

class myClass
{
      int one;
      std::string two;
      double three;
};

how to make a comparator that compares 2 myClass by first evaluating one, then two, then three.
->

// As member (inline in the class definition)
    bool operator< (const myClass& other)
    {
         return std::tie(one,two,three) < std::tie(other.one, other.two, other.three);
    }

// As global
    bool operator< (const myClass& left, const myClass& right)
    {
         return std::tie(left.one, left.two, left.three) < std::tie(right.one, right.two, right.three);
    }

[laserlight]

so if you insist on a map with pointers as key, you need a global operator that compares 2 AStarNodePair pointers, ideally though not necessarily this should be comparing const pointers.

You cannot overload operator< with both parameters of pointer type though, hence my suggestion of a named function or function object to serve as the comparator.

[lucky6969b]

std::map<AStarNodePair, float, AStarNodePairComparator> actualCosts;

bool AStarNodePairComparator::operator()(const AStarNodePair& one, const AStarNodePair& two) const {
	return std::tie(one.from, one.to) < std::tie(two.from, two.to);
}

 
float Grid::getActualCost(int fromX, int fromZ, int toX, int toZ) {		 	 
	 
	try {
		AStarNode* from = grid[fromX][fromZ];
		AStarNode* to = grid[toX][toZ];
		AStarNodePair np(from, to);
		float cost = actualCosts.at(np);
		return cost;
	}
	catch (...) {
		return -1.0f;
	}
	
}

The comparator isn't caught by the debugger. BTW, how do I compare the values by operator()
it returns a boolean saying if the first is smaller than the second, but they don't test for equality.
Thanks
Jack

[monarch_dodra]

The comparator isn't caught by the debugger. BTW, how do I compare the values by operator()

Not sure what that means. But your code seems code but it doesn't show everything. It *should* word.

BTW, what you are comparing here is not actually pointers, so you could just define a global operator<

it returns a boolean saying if the first is smaller than the second, but they don't test for equality.

Indeed. Orderable containers don't have a notion of "equality" but of "equivalence". Two objects are said to be equivalent if neither is smaller than the other. For example, if you are ordering pairs according to their norm: (1, 2) and (2, 1) are not equal, but they are equivalent in the sense that they have the same norm, and as such, equivalent position in terms of ordering.

[lucky6969b]

Hi,
Now I've added a global operator of <, what's next how to make use of it?

bool operator < (const AStarNodePair& one, const AStarNodePair& two) {
	return std::tie(one.from, one.to) < std::tie(two.from, two.to);
}

As far as I know, if I define a struct of operator (not global operators), I can put it inside the container declaration.
like

std::set<AStarNodePair, float, AStarNodePairComparator> actualCosts;

Now I don't have that wrapper, how can it be applied to the set?
BTW,
when you say that my previous work should be working, I don't understand why the actualCosts.at(np);
statement reaches the catch clause... quite strange. I also assume it works that way too.
Thanks
Jack

[laserlight]

Since you chose to overload operator< for AStarNodePair, you no longer need AStarNodePairComparator. However, I notice that you suddenly switched to std::set.

[lucky6969b]

Compilation Error in my program for sure!
But I just put the operator in my .cpp file, it will automatically compare for me...
No need to specify to use this operator?
Thanks

[D_Drmmr]

float Grid::getActualCost(int fromX, int fromZ, int toX, int toZ) {		 	 
	 
	try {
		AStarNode* from = grid[fromX][fromZ];
		AStarNode* to = grid[toX][toZ];
		AStarNodePair np(from, to);
		float cost = actualCosts.at(np);
		return cost;
	}
	catch (...) {
		return -1.0f;
	}
}

It's not idiomatic in C++ to use exceptions for control flow. Instead of calling at and catching any exception, call find and compare the returned iterator to actualCosts.end(). That way you can reserve the use of exceptions for more exceptional situations (pun intended).