3

Okay, not sure what I'm doing here, other than it's not right. Trying to overload the '==' method of a class, and it's just... not working. At least, I get a false back from my main, and the cout in the implementation of '==' doesnt output.

These are my three files:

// TestClass.h #ifndef TESTCLASS_H #define TESTCLASS_H class TestClass { public: TestClass(int contents); TestClass(const TestClass& orig); virtual ~TestClass(); bool operator==(const TestClass& other); private: int contents; }; #endif /* TESTCLASS_H */ // TestClass.cpp #include <iostream> #include "TestClass.h" TestClass::TestClass(int contents) { this->contents = contents; } TestClass::TestClass(const TestClass& orig) { this->contents = orig.contents; } TestClass::~TestClass() { } bool TestClass::operator ==(const TestClass& other) { std::cout << "COMPARING" << std::endl; return (contents == other.contents); } // Main.cpp #include <cstdlib> #include <iostream> #include "TestClass.h" using namespace std; /* * */ int main(int argc, char** argv) { TestClass* tc = new TestClass(1); TestClass* tc1 = new TestClass(1); cout << (tc == tc1) << endl; return 0; } 

So the question is - what have I done wrong? I apologise for what is probably a very silly mistake somewhere, but I just can't spot it.

6
  • 2
    You should prefer an initialization list over assignment in the constructor, you don't need to define the copy-constructor (again, use an initialization list) because the implicitly created one will copy each member anyway, and you shouldn't define an empty destructor. Commented Aug 19, 2010 at 22:15
  • @GMan: +1 on your comment but in his defence the destructor is marked as virtual so it needs to be defined. Whether the class actually needs a virtual destructor is another question entirely of course... :) Commented Aug 19, 2010 at 22:29
  • Sorry, the destructor was just generated by netbeans when I auto-generated the test class. Wouldn't normally be there =). Commented Aug 19, 2010 at 22:39
  • @Troubadour: Oops, you're right, didn't see it. Though I doubt it needs to be virtual. (Dumb IDE's forcing bad practice. If I need a destructor I'll write it my damn self.) Commented Aug 19, 2010 at 22:53
  • @Stephan, you don't need to use new to create an instance of a variable. This is a technique that is used in Java. Just declare your variables as TestClass tc; TestClass tc1;. Until you get experience, only use new for huge objects; pass everything by reference or const reference. Commented Aug 19, 2010 at 23:16

2 Answers 2

11

tc == tc1 compares pointer values. It "should" be *tc == *tc1, but I don't get why you'd dynamically allocate in the first place.

Automatic (stack) allocation is highly preferred, only dynamically allocate when you need the object to be independent of scope. (And then keep track of it with automatically allocated smart pointers, which will delete the pointer when it's appropriate.)


Also, the operator should be const, because it doesn't modify this:

// vvvvv bool operator==(const TestClass& other) const; 

Even better, though, is a free function:

bool operator==(const TestClass& lhs, const TestClass& rhs); 

Which would possibly be a friend. (Free-functions are always preferred, plus this allows 5 == tc to work.)

Sign up to request clarification or add additional context in comments.

11 Comments

Thanks for the answer. I really should have spotted the pointer problem, sorry. However, your answer did also teach me other things, and leave me with a few questions - mostly about this 'dynamic' versus 'automatic' allocation. To be frank, I have no idea what you're talking about - google for 'C++ automatic allocation' gives a first result that claims dynamic and automatic allocation are the same thing (cplusplus.com/forum/articles/416). So, sorry, but what are you actually talking about in regards to that?
@Stephen, what GMan is referring to that instead of saying TestClass* tc = new TestClass(1); you can also write TestClass tc(1); and it will construct an object with the same value. This is the preferred way in C++ over new/delete. You also missed out on the delete for tc and tc1, so these objects will never be destroyed. C++ doesn't have a garbage collector like C# or Java does, you have to take out the garbage yourself.
@Stephen: That link makes no sense (that person obviously fails to understand the difference between C++ the language and implementations). I wish there were a super-delete button on the internet. In any case, dynamic, automatic, and static are storage durations. An automatic is the default, and objects with automatic duration will be destroys automatically (hence the name) at the end of their scope. Dynamic allocation is independent from scope, and you obtain a pointer to a dynamically allocated object via new (and friends.) If you never manually delete a dynamically allocated object...
@Stephen I recommend: stop. Read a book about C++. You will not get this stuff right by trial and error.
...its lifetime will never end, resulting in a leak. (You can end the lifetime of a dynamically allocated object via delete (and friends.)) In C++, automatically allocated objects are (on a typical platform) quickly allocated because they are on the stack, and much safer. (Impossible to leak!) Contrarily, dynamic allocation tends to be slower, and is easy to mess up. Modern C++ uses automatic objects to "own" objects, and will automatically free them when it's time. (They pass ownership around, or share it.) In any case, you need to start from the beginning. Get a good book and read.
|
4

You are comparing pointers. Try that instead:

cout << (*tc == *tc1) << endl; 

Two remarks:

  • You should free allocated memory with delete, or use a smart pointer
  • You should declare operator== const:

    bool operator==(const TestClass& other) const

1 Comment

Two more remarks: * operator== should be a non-member function, and * good luck implementing operator== with dynamic polymorphism: drdobbs.com/184405053

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.