tags:

views:

683

answers:

5

I'm trying to determine whether an object is already contained within a std::set. According to msdn (and other sources) the set::find function is supposed to return end() if it doesn't find the element you asked for.

However when I implement code like the following, set::find returns junk (0xbaadf00d) instead.

set<Cell*> cellSet;

Cell* cell = new Cell();    

if (cellSet.find(cell) == cellSet.end())
{
    ...
}

Am I using this correctly? I'm working in Visual C++ 2005.

+1  A: 

Does callSet.end() have the value 0xbaadf00d as well?

EDIT

I ran this sample code in VS2008 and everything worked as expected. The find function returned an iterator pointing to the original value.

What behavior exactly are you seeing? Does it return end() or does it return another place in the set?

JaredPar
It does, but it crashes if I try to do an == between them, and I can't depend on that value anyway because it's just a debugging flag. In release mode they would contain two random values.
Kevin Laity
It returns the proper iterator if it finds the object, and an iterator pointing to 0xbaadf00d if it doesn't
Kevin Laity
@Kevin, as long as end() == 0xbaadf00d this should be the correct behavior.
JaredPar
@Kevin: Wait a minute -- are you still doing something with the return value of find() when it doesn't find anything?
j_random_hacker
@j_random_hacker: no I'm not doing anything with it but comparing it to end. It turns out my code works fine, and the crash was coming from elsewhere.
Kevin Laity
+10  A: 

Your code as posted will always execute the code within the if, and 0xbaadf00d is the implementation's "one-past-the-end" marker.

David Schmitt
Yeah apparently it is, apparently the crash was coming from somewhere else because after moving some stuff around, it seems to work fine now.
Kevin Laity
The fact that it's pronounceable and almost meaningful suggests a deliberate bad value. 0xdeadbeef was popular as a canonical bad or uninitialized value for that reason.
David Thornley
Don't you mean that the code as posted should *always* execute the code within the if? cellSet is empty (no inserts were performed in the posted code) and cell is new (cannot possibly be in cellSet even if an insert had been performed) so cellSet.find(cell) should *always* return cellSet.end(). Just trying to be clear.
Naaff
@Naaff: in the original post, there was an cellSet.insert(cell) inbetween. I'll adjust my answer.
David Schmitt
A: 

Try compiling and running just the code snippet you provided, and I guarantee you will find it executes without problems. The problem is almost certainly due to memory allocation bugs occurring elsewhere in your program, such as referencing an uninitialised pointer or a pointer to an object that has been deleted.

Are you familiar with how C++ containers manage their objects? They don't delete pointers for you. It is always safer to use containers of objects, rather than pointers to objects, when possible. (There are cases when containers of pointers are necessary -- in particular, when you want the container to store objects of different types from a single class hierarchy.)

j_random_hacker
+4  A: 

When using stl set I like to use the count function to determine membership. I think this makes the code easier to read.

set<Cell*> cellSet;

Cell* cell = new Cell();    

if (cellSet.count(cell) == 0)
{
    ...
}
Alex B
Great approach, why didn't I think of that? :)
Kevin Laity
Calling count() requires examining every element, while find() examines every element only when the element is not present. So find() will be faster. If readability is a concern, it is better to write an inline function is_member that takes an Associative Container and an element and returns find() == end().
Mark Ruzon
@Mark Ruzon: for std::set, both have logarithmic complexity. In fact, for GNU stdlibc++, the implementation of count is basically: { return find(x) == end() ? 0 : 1; } http://gcc.gnu.org/viewcvs/trunk/libstdc%2B%2B-v3/include/bits/stl_set.h?view=markup
Miles
What Mark says would be true for a multiset, but this is a set. So I guess it depends whether the code in question is written with an eye to be generalised beyond just sets.
Steve Jessop
I believe Scott Meyers recommends this approach in Effective STL, too
James Hopkin
+1  A: 

One simple mistake is that you should be testing for not equal to end.

set<Cell*> cellSet;
Cell* cell = new Cell();
if (cellSet.find(cell) != cellSet.end())     // Test NOT EQUAL to end
{
     // Found item in set.
}

But also you should note that you are not comparing the actual Cell values but the pointer to Cell objects (which may or may not be what you wanted). Normally in C++ you don't tend to store pointers in containers as there is no implied ownership of the pointer but somtimes that is OK.

To actually compare objects you need to use the find_if() and pass a predicate (a functor).

struct PointerCellTest
{
    Cell&  m_lhs;
    PointerCellTest(Cell* lhs): m_lhs(lhs) {}
    bool operator()(Cell* rhs)
    {
         return lhs.<PLOP> == rhs.<PLOP>
    }
};


if(find_if(cellSet.begin(),cellSet.end(),PointerCellTest(cell)) != cellSet.end())
{
     // Found item in set.
}
Martin York
In this case the set is being used very temporarily so it should be fine. If it were more of a long term thing, it would have been set<Cell>. I didn't know about find_if, so thank you! I will remember that for next time.
Kevin Laity