views:

565

answers:

10

Often I add an Empty method to my C++ objects to clear the internal state using code similar to the following.

class Foo
{
private:
    int n_;
    std::string str_;
public:
    Foo() : n_(1234), str_("Hello, world!")
    {
    }

    void Empty()
    {
        *this = Foo();
    }
};

This seems to be better than duplicating code in the constructor, but I wondered if *this = Foo() is a common approach when wanting to clear an object? Are there any problems with this waiting to bite me on the backside? Are there any other better ways to achieve this sort of thing?

+13  A: 

I'd let the constructor call my function instead:

class Foo
{
private:
    int n_;
    std::string str_;
public:
    Foo()
    {
        Reset();
    }

    void Reset()
    {
        n_ = 1234;
        str_ = "Hello, world!";
    }
};

Yes, you're unnecessarily initializing the string as an empty string first, then doing an assignment, but this is much clearer.

Ates Goral
But what would you do if `Foo` inherits from `Bar` which does not have a default constructor and would need to be initialized using the initialization list?
paracycle
If Bar doesn't need to be resettable as well, it would just be initialized at construction: `Foo() : Bar(DUMMY) { Reset(); }` If Bar needs to be resettable, I'd assume that it would have a Reset function as well: `void Reset() { Bar::Reset(); ... }`. There could therefore be some redundant initialization of Bar members, for the sake of clarity.
Ates Goral
Good point. I like this approach for non-performance critical applications.
paracycle
+5  A: 

Also, consider making objects immutable, i.e., when constructed, they cannot be changed. This can in a lot of scenarios save yourself from unanticipated side effects.

Johann Gerell
+1  A: 

Hi Rob,

consider using placement new:

void Empty() {
    this->~Foo();
    new (this) Foo();
}

Your code invokes operator = which might lead to all kinds of side-effects.

EDIT in response to the comments. – This code is definitely well-defined, the standard explicitly allows it. I'm going to post the paragraph later if I find the time. About delete – of course. What I meant was ~Foo(), this was an oversight. And yes, Rob is right as well; destructing the object is actually necessary here to call the string's destructor.

Konrad Rudolph
Oh my. That looks like a REALLY bad idea. You just deleted the this pointer (i.e. you just returned that memory to the memory manager). If you're lucky, you'll crash quickly. If not, you'll just give corrupted state to who knows what part of your program. Don't do that.
Michael Kohne
You meant "this->~Foo()" not "delete this".
Greg Rogers
Even without deleting "this," it's still wrong. You're calling the constructor on already-initialized memory. The std::string field of the object will get overwritten with a new one; its destructor won't get called first.
Rob Kennedy
Thanks for the comments. The code was botched up, I've corrected it now. In my defense, I had initially understood the question wrong, then re-read it and edited my posting. Before that initial edit, the code was actually right (the same as it is now!), my edit I disimproved it.
Konrad Rudolph
herb sutter doesn't recommend this: http://www.gotw.ca/publications/advice97.htm . that said - yes it *is* well defined, but will fail hard once you derive from Foo, for example. herb knows stuff better than me and collected other problems.
Johannes Schaub - litb
and yes, even without executing the dtor manually - if the dtor has no side effects you depend on, it's valid to omit the call (but then, it's even worse :)) - no UB there either. although in this case - you would omit calling strings dtor, where you can't guarantee that it causes not UB.
Johannes Schaub - litb
@litb: I vaguely remember us having the discussion about the dtor before. You're of course right about derived classes. However, since others in this thread have already pointed out this problem, I don't think I need to update my answer to reflect this, as well.
Konrad Rudolph
Konrad, yeah your answer is fine as is, indeed. just wanted to tell the crowd what i think the state of the matter is, hehe :)
Johannes Schaub - litb
A: 

Yes, this is not efficient in terms of performance (creating another foo object instead of working in place) and it will bite you if you'd allocate memory in the constructor with a nasty memory leak.

Making it safer in terms of memory would be calling the this->delete and this = new foo() - but it will be SLOW.

if you want to be superfast create a static empty object field and memcpy it in Reset.

if you want to be just fast assign the properties one by one.

if you want to maintain reasonable style without duplication call Reset from Ctor as Ates Goral suggested but you'll lose the faster construction with the default params.

Dani
Memcpy leads to undefined behavior on non-POD types. And allocating a new object and assigning the result to "this" certainly won't do what you intend.
Rob Kennedy
+9  A: 

Potential problems? How do you know that *this really is a Foo?

William
+4  A: 

There is something even more common than what you have suggested. using swap.

Basically you do something like this:

T().swap(*this);

since many standard containers (all of the STL containers?) have a constant time swap method, this is a nice and simple way to clear a container and make sure it's storage is released.

Similarly, it is a good way to "shrink to fit" a container as well, but using the copy constructor instead of the default constructor.

Evan Teran
yeah that's what i wanted to post now hehe. +1
Johannes Schaub - litb
+6  A: 

What you're doing with this Empty method, is essentially the same as manually assigning a newly constructed object to a variable (the thing that the Empty function does).

Personally, I'd remove the Empty method, and replace all uses of that method with this:

// let's say, that you have variables foo and pfoo - they are properly initialized.
Foo foo, *pfoo;

// replace line "foo.Empty()" with:
foo = Foo();

// replace line "pfoo->Empty()" with:
delete pfoo;
pfoo = new Foo();
// or
*pfoo = Foo();

I really see no advantage in having this Empty method. It hides what is really happening to the object on witch it's called, the name isn't the best choice either.

If the caller really wants a clean object - he will have no problem constructing the object himself.

Paulius Maruška
Exactly my thought. This "object recycling" is not going to help in any way and may make maintenance and debugging harder.
Nemanja Trifunovic
Perhaps 'Clear' would of been a better name - many STL types have a 'clear' method.
Rob
The stl objects that have a clear method are container classes that can "cheat" in clearing by not freeing up memory.
Eclipse
Why delete pfoo when you can just "*pfoo = Foo();"
Greg Rogers
Good call Greg, I added it as an alternative. Thanks.
Paulius Maruška
+1  A: 

This seems to be better than duplicating code in the constructor, but I wondered if *this = Foo() is a common approach when wanting to clear an object?

Clearing an object isn't that common a thing to do: more commonly, either an object (perhaps even an immutable object) is instantiated and contains real data, or it isn't instantiated.

The most common kind of thing that are reset would be containers ... but, you wouldn't be writing your own container classes now, would you.

Are there any problems with this waiting to bite me on the backside?

Yes:

  • If this isn't really a Foo but is instead a DerivedFoo
  • If Foo's assignment operator doesn't exist, or if it's buggy (e.g. if it's not defined and the default operator isn't good, e.g. because of a data member is a naked pointer).

Are there any other better ways to achieve this sort of thing?

Yes, maybe a free function would be better (would avoid both the above problems):

template<class T> void reconstruct(T* p)
{
    p->~T();
    new (p) T();
}
ChrisW
+2  A: 

This can be a potential source of memory leak if you have a dynamically allocated memory in the constructor.

Sridhar Iyer
+2  A: 

Here's how i do it:

class Foo {
private:
    int n_;
    std::string str_;
public:
    Foo() : n_(1234), str_("Hello, world!")
    {
    }

    void Empty()
    {
        Foo f;
        swap(f);
    }

    void swap(Foo & other) {
        std::swap(n_, other.n_);
        swap(str_, other.str_);
    }
};

void swap(Foo & one, Foo & other) {
    one.swap(other);
}

Put the swap function into the same namespace like the Foo class. Argument dependent lookup finds it when users do a call to swap to swap two Foo. You can implement operator= using your swap function too.

Johannes Schaub - litb
How about making `swap` a `friend` of `Foo`? At the moment, it can't access the private members.
Konrad Rudolph
Konrad, oh right. i missed that :) ok so then i'll just put it as a member and call it from the free function :) that's how std::string does it. i wondered about why it does so (having that extra member). but this friend issue maybe is the key
Johannes Schaub - litb