0

The question is how to correctly delete name in A destructor?

class A{
private:
    char *name;
public:
    A(char *n) : name(n) {}
    ~A(){
        //???
    }
}

int main(){
    A *a = new A("name");
    delete a;
    return 0;
}
Jay Foreman
  • 600
  • 1
  • 6
  • 17
  • 4
    Exactly one `delete` for each `new`, exactly one `delete[]` for each `new[]`. Your program has one `new` and one `delete`. It is, therefore, complete. – Robᵩ May 14 '13 at 20:14
  • Generally, in C++, when I see a pointer as an argument to a function or constructor, I assume no ownership, unless explicitly documented. And, generally don't write types that have raw pointers. – Nathan Ernst May 14 '13 at 23:27

5 Answers5

7

Given that you do not change the constructor, then the correct way is to not delete anything. Ownership of the string belongs to the client, since you are not creating a copy.

However, the right way of rewriting this is to let the constructor allocate a copy of the string with new[] and let the destructor deallocate it with delete[].

And the really right way is to let std::string do the whole thing for you, and not write an explicit destructor at all:

#include <string>

class A{
private:
   std::string name;
public:
    A(std::string n) : name(std::move(n)) {}
};

This, by the way, allows you not to worry about the Rule of Three, which means you won't have to bother writing a copy constructor, a move constructor, a copy assignment operator, a move assignment operator, a destructor and whatnot.

Andy Prowl
  • 124,023
  • 23
  • 387
  • 451
  • 2
    I was hoping one of these would mention `std::string` as the truly correct way. – chris May 14 '13 at 20:14
  • So, am I free to leave the destructor standard in this case? – Jay Foreman May 14 '13 at 20:16
  • @user2370748: Yes and no. In the bizarre design decision to leave ownership of the pointed string to the client, then yes, you do not have define a destructor. However, what happens if the string pointed to by `name` goes out of scope? You're left with a dangling pointer! That's why you would normally create a *copy* of the string, so in the constructor you would do `new[]` and in the destructor you would do `delete[]`. BUT you can let `std::string` do all this stuff (and more!) for you, so why not using it? (Basically, this comment is a repetition of the answer) – Andy Prowl May 14 '13 at 20:19
  • @Andy I meant leaving destructor standard in your case with string... Well, I got your answer, thanks! :) – Jay Foreman May 14 '13 at 20:24
  • Could you enlighten me as to what exactly you like better (if anything) about this constructor than `A(const std::string &n) : name(n){}`? Yours looks, unless I'm counting wrong, like it has an extra move, but I suppose yours is somewhat pedantically safer in that `n` can't externally change in the extremely short time this takes to run. It shouldn't make a huge difference anyway, but the heck with it if I learn or realize something I didn't before. – chris May 14 '13 at 20:30
  • @chris: If you pass an rvalue to my constructor it will perform two moves. If you pass an rvalue (or an lvalue, for what it matters) to a constructor taking `A(const &)`, it will perform one copy. If you pass an lvalue to my constructor, on the other hand, it will perform one copy and one move vs just one copy. Since moves are O(1) and copies are O(N), the complexity is better when an rvalue is passed, and identical when an lvalue is passed. – Andy Prowl May 14 '13 at 20:34
  • @AndyProwl, I didn't even consider rvalues being passed in (how silly), thanks. – chris May 14 '13 at 20:35
  • @chris: That's more or less the same reaction I had when I've seen it done by someone else ;) – Andy Prowl May 14 '13 at 20:36
  • by the way, wouldn't it be better to have a constructor by copy, and one by move? like `A(std::string const& n) : name(n) {}` and `A(std::string && n) : name(n) {}`. so i can decide whether i want copy or move? – user1810087 May 14 '13 at 20:36
  • @itwasntpete: Well, yes, technically that's fine, but personally I would not introduce another constructor just to save on one move of a `string`. After all, moves are cheap for `string`. But yes, you could do that – Andy Prowl May 14 '13 at 20:39
  • 1
    @itwasntpete, I just rediscovered [this](http://stackoverflow.com/questions/15600499/how-to-pass-parameters-correctly/15600615#15600615). It's quite the good read. – chris May 14 '13 at 20:44
2

You are not allowed to delete a pointer to string constant "name".

As your class does not own any objects or memory blocks, it should not delete anything.

nullptr
  • 11,008
  • 1
  • 23
  • 18
1

In this example you don't need to delete name. "name" is just a pointer to some some place in executable image, not memory allocated with new/malloc/something_else.

1

"name" occupies static memory that the compiler automatically sets aside. If you delete that memory, the behavior is undefined (i.e. crash). You only 'delete' memory that you 'new' in the first place.

edtheprogrammerguy
  • 5,957
  • 6
  • 28
  • 47
1

The simplest thing is to use proper C++ (std::string) and avoid naked pointers, as that simplifies management of resources.

The problem with your interface is that the type A claims ownership of the argument, but you cannot claim ownership of a string literal, as those are managed by the implementation. You could go on trying to decide whether the caller should create deep copy and pass it to a A or if A design should be changed not to attempt to claim ownership and make a copy out of that. But the fact is that things are much simpler if you just use higher level constructs:

class A {
   std::string name;
public:
   A(const std::string& name) : name(name) {}
};

No need to manually copy data around, no need for a destructor or a copy constructor... all works out of the box.

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489