1

I'm having a hard time with current project. In previous versions I used std::vectors and all went fine, no memory leaks, no errors, just fine.

Then I switched to use some pointers since it's just more elegant and I have improved performance a bit.

So I have classes like

class A
{
    private:
        std::string str;
    public:
        A() {str = "";}
};

class B
{
    private:
        std::string str;
        A* a;
    public:
        B() {str = ""; a = NULL; }
};

class C
{
    private:
        std::vector<B*> bs;
    public:
        C() { bs = std::vector<B*>(); }

};

I know that every time I use new, I have to delete it afterwards. So I create destructors for class B and class C.

That's what I think it should look like:

B::~B()
{
    if ( this->a != NULL )
        delete this->a;
}

C::~C()
{
    while ( (this->bs).size() > 0 )
    {
        if ( (this->bs).back() != NULL )
            delete (this->bs).back();
        (this->bs).pop_back();
    }
    (this->bs).clear(); // not necessary since bs has size 0?
}

But with that, I'm getting various errors, such as "Invalid read of size 4" in valgrind.

Is my idea of what destructors should look like ok? What else could be problematic with my code? Would it be good to change the private std::string members to std::string-pointers? Is it necessary or a good thing to do str = ""; in destructors?

On a side note: Please understand that I have done a lot of searching on "proper destructors" and similar keywords and it just didn't help. If you think this was asked too often: I don't, because I don't get it.

stefan
  • 10,215
  • 4
  • 49
  • 90
  • 5
    Ah ha ha ha - I think your idea of elegance is slightly flawed! As for efficiency... did you do the performance tests to prove this increase?? – Nick May 11 '12 at 14:21
  • 3
    "I used std::vectors and all went fine". Yes, that's generally the case with `std::vector`. What does that tell you? – Robᵩ May 11 '12 at 14:22
  • I don't need a special test ;-) The performance improve was roughly 3x faster. – stefan May 11 '12 at 14:22
  • `delete (this->bs).back();` "deletes" an iterator, not the pointer. – RageD May 11 '12 at 14:23
  • There isn't a single use of the new operator in your example. It is not possible for us to know when you should use delete. It's also worth pointing out that standard containers are designed specifically so you can avoid this hassle. – luke May 11 '12 at 14:23
  • 1
    I very much doubt that you have gained a 300% increase in performance just by using `new`/`delete` rather than stl containers and smart pointers. – Nick May 11 '12 at 14:25
  • @RageD - I disagree. `.back()` doesn't return an iterator, it returns a reference to an element of the vector. – Robᵩ May 11 '12 at 14:27
  • Various other small changes might have supported this increase in performance as well, but the major change was the change to pointers – stefan May 11 '12 at 14:27
  • 1
    @stefan - You have provided a very incomplete sketch of your program. If you want to receive answers that are better than guesses, please provide a **short**, **commplete** program that demonstrates the problem. See http://sscce.org/. – Robᵩ May 11 '12 at 14:28
  • 1
    'Is it necessary or a good thing to do str = ""; in destructors?' No, it's not necessary. It's also not necessary in the constructor. str was already default-constructed. – Henrik May 11 '12 at 14:33
  • @Robᵩ in what way is this incomplete? the only thing "missing" is the main function. But it's fairly obvious how that function would look like – stefan May 11 '12 at 14:34
  • 1
    *Aside*: You don't need `A::A()`; the default constructor has identical effect. Your `B::B()` is odd; try `B() : a(NULL) {}` instead. You don't need `C::C()`; the default constructor has identical effect. – Robᵩ May 11 '12 at 14:34
  • 2
    It is missing `main`, `#include`s, and any mention of the `new` operator. It is missing adding any elements to your `vector`. Even after adding `main`, this program *won't produce the error you complain about.* – Robᵩ May 11 '12 at 14:36
  • It is difficult to be certain what is wrong, since you have not provided a complete sample program. I suspect that you have violated the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). In short, if you expect to use naked pointers safely, you must also implement a copy constructor and a copy assignment operator. On the other hand, if you use smart pointers or containers, you don't need to worry about this. – Robᵩ May 11 '12 at 14:22

1 Answers1

1

What Rob said, and also your destructors can be simplified to:

B::~B()
{
    delete a;
}

C::~C()
{
    for (size_t i = 0; i < bs.size(); ++i)
        delete bs[i];
}
Andreas Brinck
  • 51,293
  • 14
  • 84
  • 114
  • why isn't it necessary to check for != NULL? i thought deleting the NULL-pointer were illegal. – stefan May 11 '12 at 14:28
  • Quoting the C++2003 standard, §5.3.5/2, "if the value of the operand of delete is the null pointer the operation has no effect." – Robᵩ May 11 '12 at 14:30
  • Just a nit: _formally_ (and I insist on the formally), your destructor for `C` has undefined behavior. (Practically, it will work in all implementations, and it's one of the rare cases of undefined behavior that I don't worry about.) – James Kanze May 11 '12 at 14:37
  • 1
    @James Do you mean mine or the OP's? What's undefined about it? – Andreas Brinck May 11 '12 at 14:38
  • 1
    @AndreasBrinck Yours (and the OP's). The standard says that all values in an `std::vector` must be copyable, and a pointer to deleted memory isn't copyable. Of course, 1) no implementation is actually going to copy the pointer unless you try to do something with it (or increase the size of the `vector`), and 2) implementations where copying a pointer to a deleted object will not work are inexistant, at least today. (The restriction dates from the early days of C, where certain implementations on Intel 80286 could fault when copying a deleted pointer.) – James Kanze May 11 '12 at 14:45
  • @James So you mean that to avoid UB the pointer needs to be removed from the vector before being deleted? You're right I won't worry about this either :) – Andreas Brinck May 11 '12 at 14:50
  • @James Just for completeness, do you have the paragraph from the standard which states that a deleted pointer is non copyable? – Andreas Brinck May 11 '12 at 14:53
  • @AndreasBrinck Yes. Either remove the pointer from the container first, or replace it with a null pointer in the container. A simple way to do this would be `for_each` with a functional object `struct Deleter { void operator()( T*& p ) const { T* tmp = NULL; std::swap( tmp, p ); delete tmp; };`. But frankly, I don't think it's worth the bother. (And the relevant paragraph in the standard is §3.7.4.2.) – James Kanze May 14 '12 at 08:01
  • @James I can't find paragraph 3.7.4.2, at least not in the ISO 14882 (2003) version of the standard, typo? – Andreas Brinck May 15 '12 at 07:00
  • @AndreasBrinck It's 3.7.4.2 in C++11; 3.7.3.2 in C++03. (I should have specified which I was citing. Especially since I usually use C++03.) The exact quote is the same in both versions: "[...] the deallocation function shall deallocate the storage referenced by the pointer, rendering invalid all pointers referring to any part of the deallocated storage. The effect of using an invalid pointer value [...] is undefined." The traditional interpretation of "pointer _value_" is that it is the results of an lvalue to rvalue conversion. – James Kanze May 15 '12 at 07:37