constructor and reset method

[Christopher]

Embarrassed to ask, but is there any functional difference between

class A
{
int m_x;
int m_y;

public:
A()
:
m_x(0),
m_y(0)
{
}

~A(){}

Reset()
{
m_x = 0;
m_y = 0;
}
};


class B
{
int m_x;
int m_y;

public:
B()
:
m_x(0),
m_y(0)
{
}

~B(){}

Reset()
{
B();
}
};


Both seem to compile and work as expected, but as I've learned over
the years, just because it runs and shows you what you want to see,
doesn't mean it works.

 

[Jeff Schwab]

Embarrassed to ask, but is there any functional difference between

[code like:]

class A {
int m_x;
int m_y;

public:
A() : m_x(0), m_y(0) { }

void Reset() {
m_x = 0;
m_y = 0;
}
};

class B {
int m_x;
int m_y;

public:
B(): m_x(0), m_y(0) { }

void Reset() { B(); }
};

Both seem to compile and work as expected, but as I've learned over
the years, just because it runs and shows you what you want to see,
doesn't mean it works.

Smart man.

The method B::reset is not invoking the constructor on the right object.
It is only creating a temporary object of type B, and in no way
affecting the object on which Reset was called.

Your A::Reset approach is generally the right way to do this.

 

[Alf P. Steinbach]

* Christopher:

Embarrassed to ask, but is there any functional difference between

class A
{
int m_x;
int m_y;

public:
A()
:
m_x(0),
m_y(0)
{
}

~A(){}

Reset()
{
m_x = 0;
m_y = 0;
}
};


class B
{
int m_x;
int m_y;

public:
B()
:
m_x(0),
m_y(0)
{
}

~B(){}

Reset()
{
B();
}
};


Both seem to compile and work as expected, but as I've learned over
the years, just because it runs and shows you what you want to see,
doesn't mean it works.

B::Reset doesn't do anything.

What you have is a construction of a temporary.

That isn't a bad idea in and of itself (although it's probably not
what you intended), but it needs a little more support machinery:

class B
{
int myX;
int myY;
public:
B(): myX(0), myY(0) {}

reset() { *this = B(); }
};

Cheers, & hth.,

- Alf

 

[Alf P. Steinbach]

* Alf P. Steinbach:

* Christopher:

B::Reset doesn't do anything.

What you have is a construction of a temporary.

That isn't a bad idea in and of itself (although it's probably not
what you intended), but it needs a little more support machinery:

class B
{
int myX;
int myY;
public:
B(): myX(0), myY(0) {}

reset() { *this = B(); }
};

Hm. Add a void.

 

[Jeff Schwab]

* Christopher:

reset() { *this = B(); }

I've never seen that before, but it does seem like a neat idea.

It seems like it would be (generally speaking) twice as slow as manual
member-by-member initialization, because of (a) the temporary object's
constructor, plus (b) the assignment operator.

 

[jason.cipriani]

class B
{
int myX;
int myY;
public:
B(): myX(0), myY(0) {}

reset() { *this = B(); }
};

Sometimes I go the other way around, as long as B is simple enough
that you can call Reset() on an unitialized B:

class B
{
int myX;
int myY;
public:
B() { Reset(); }
void Reset () {
myX = 0;
myY = 0;
}
};

But if you have to deallocate memory or something you still need to
initialize some things in the constructor:

class B
{
int *myInts;
public:
B() : myInts(NULL) { Reset(); }
void Reset() { // or something to this effect...
delete[] myInts;
myInts = NULL;
}
};

....and that all breaks down and turns pretty useless anyway once B
starts having members that are other complex objects (that you'd also
want to "Reset").

Jason

 

[jason.cipriani]

I've never seen that before, but it does seem like a neat idea.

It seems like it would be (generally speaking) twice as slow as manual
member-by-member initialization, because of (a) the temporary object's
constructor, plus (b) the assignment operator.

Just now I managed to get this working using placement new. Is there
anything bad that can happen with the following code (assuming the
constructor does not throw any exceptions):

class B {
public:
B () { }
~B () { }

void Reset () {
this->~B();
new (this) B();
}

};

I've done a good amount of testing on that just now and it seems to
work fine (even when B has other complex object members, etc.).
Assuming it's safe, it doesn't require you to implement an assignment
operator for complex B's, and doesn't create any temporaries. It seems
like it's just a different way of expressing the following, with the
addition of automatically calling member object destructors and
constructors:

class B {
public:
B () { Init(); }
~B () { Cleanup(); }
void Reset () {
Cleanup();
Init();
}
private:
// these do things:
void Init () { ... }
void Cleanup () { ... }
};

Jason

 

[Jeff Schwab]

Just now I managed to get this working using placement new. Is there
anything bad that can happen with the following code (assuming the
constructor does not throw any exceptions):

class B {
public:
B () { }
~B () { }

void Reset () {
this->~B();
new (this) B();
}

};

I've done a good amount of testing on that just now and it seems to
work fine (even when B has other complex object members, etc.).
Assuming it's safe, it doesn't require you to implement an assignment
operator for complex B's, and doesn't create any temporaries. It seems
like it's just a different way of expressing the following, with the
addition of automatically calling member object destructors and
constructors:

class B {
public:
B () { Init(); }
~B () { Cleanup(); }
void Reset () {
Cleanup();
Init();
}
private:
// these do things:
void Init () { ... }
void Cleanup () { ... }
};

The primary thing I would be concerned about is overwriting any
subobjects of B. If B has non-POD members, they may not take kindly to
be reconstructed in place, without any chance to delete resources they
were already holding.

For example: If B has a std::vector<int> member, the vector likely has
a pointer to some dynamically allocated memory. By using placement new,
rather than emptying the vector through its intended interface, you may
be overwriting the vector's pointer to its dynamically allocated memory,
such that the memory is leaked.

 

[Alf P. Steinbach]

* (e-mail address removed):
No. Thinking about that would be premature optimization. Actually a 'reset'
member function indicates of itself premature optimization, the idea of reusing
an object to avoid some imagined micro-inefficiency, thereby complicating the
design.

For optimization, always first measure.

Just now I managed to get this working using placement new. Is there
anything bad that can happen with the following code (assuming the
constructor does not throw any exceptions):

class B {
public:
B () { }
~B () { }

void Reset () {
this->~B();
new (this) B();
}

};

I've done a good amount of testing on that just now and it seems to
work fine (even when B has other complex object members, etc.).
Assuming it's safe,

It isn't safe, in general.

In particular it's problematic if anyone ever derives from B: then you're
constructing the wrong kind of object.

To reuse the constructor logic e.g. in assignment operator, instead add a swap()
member function.

it doesn't require you to implement an assignment
operator for complex B's,

Assignment operator or not has nothing to with the above issue.

and doesn't create any temporaries.

Temporaries are not inefficient. They're not ungood. You should not focus on
avoiding temporaries.


Cheers, & hth.,

- Alf

 

[Triple-DES]

Just now I managed to get this working using placement new. Is there
anything bad that can happen with the following code (assuming the
constructor does not throw any exceptions):

class B {
public:
  B () { }
  ~B () { }

  void Reset () {
    this->~B();
    new (this) B();
  }

};

I've done a good amount of testing on that just now and it seems to
work fine (even when B has other complex object members, etc.).
Assuming it's safe, it doesn't require you to implement an assignment
operator for complex B's, and doesn't create any temporaries. It seems
like it's just a different way of expressing the following, with the
addition of automatically calling member object destructors and
constructors:

class B {
public:
  B () { Init(); }
  ~B () { Cleanup(); }
  void Reset () {
    Cleanup();
    Init();
  }
private:
  // these do things:
  void Init () { ... }
  void Cleanup () { ... }

};

Jason

class B
{
public:
void Reset()
{
this->~B();
new (this) B();
}
virtual ~B() {}
};

class D : public B
{
// ...
};

int main()
{
B * b = new D; // Ok, *b is of type D
b->Reset(); // Oops, *b is of type B
}

DP

 

[jason.cipriani]

For example: If B has a std::vector<int> member, the vector likely has
a pointer to some dynamically allocated memory. By using placement new,
rather than emptying the vector through its intended interface, you may
be overwriting the vector's pointer to its dynamically allocated memory,
such that the memory is leaked.

Hmm... but in this case, wouldn't calling this->~B() also invoke the
std::vector<int> member's destructor; which would free any memory the
vector had allocated? The memory used for the vector<int> itself would
stick around, of course (it's been destructed but not deallocated),
but any cleanup it did in it's destructor would happen. At least,
that's what I expect; because you can use placement new on object
types that have other objects as members, and the only thing you need
to do to clean up (aside from actually flagging the memory you used
for it as free in whatever way is appropriate) is call the object's
destructor.

But...

In particular it's problematic if anyone ever derives from B: then you're
constructing the wrong kind of object.

.... that makes sense. Oh well... guess that kills that idea (Triple-
DES wrote the same thing)! Thanks for pointing that out.

Assignment operator or not has nothing to with the above issue.

In the above case, no, since the default assignment operator works.
But if you say "(*this) = B()", and B is something like, say:

class B {
// ...
int *some_array_that_i_allocated;
};

Then wouldn't you have to make sure you implemented a proper
assignment operator that let "(*this) = B()" free
"some_array_that_i_allocated" first?

Jason

 

[Alf P. Steinbach]

* (e-mail address removed):

... that makes sense. Oh well... guess that kills that idea (Triple-
DES wrote the same thing)! Thanks for pointing that out.


In the above case, no, since the default assignment operator works.
But if you say "(*this) = B()", and B is something like, say:

class B {
// ...
int *some_array_that_i_allocated;
};

Then wouldn't you have to make sure you implemented a proper
assignment operator that let "(*this) = B()" free
"some_array_that_i_allocated" first?

Assuming it was more realistic (the above easily fixed using std::vector): yes.

Check out the swap idiom for implementing assignment operator in terms of copy
constructor.


Cheers, & hth.,

- Alf

 

[Juha Nieminen]

reset() { *this = B(); }

Btw, a bit related to that, can anyone spot some problem with this:

A::A(const A& rhs)
{
*this = rhs;
}


A& operator=(const A& rhs)
{
// Tons of member variable assignments here
}

The idea would be, of course, to save writing all the member variable
initializations twice.

(I really wish there was a way to call the compiler-generated "default
copy constructor" and the "default assignment operator" from your own
versions, so that you only had to concentrate on those members for which
the default assignment is not enough. The problem with having to
construct/assing every single member manually is that if you add a new
member variable in the future you have to remember to add it to both the
copy constructor and assignment operator. If you forget, the compiler
won't give you even a warning, and the program may malfunction in
strange ways, which will sometimes be very hard to debug.)

 

[Alf P. Steinbach]

* Juha Nieminen:

Btw, a bit related to that, can anyone spot some problem with this:

A::A(const A& rhs)
{
*this = rhs;
}


A& operator=(const A& rhs)
{
// Tons of member variable assignments here
}

The idea would be, of course, to save writing all the member variable
initializations twice.

It is again a problem with derived classes, but here "only" with efficiency and
expectations of maintainance programmers (the latter the most serious).

Another problem is that it requires data members to be default-constructible.

(I really wish there was a way to call the compiler-generated "default
copy constructor" and the "default assignment operator" from your own
versions, so that you only had to concentrate on those members for which
the default assignment is not enough. The problem with having to
construct/assing every single member manually is that if you add a new
member variable in the future you have to remember to add it to both the
copy constructor and assignment operator. If you forget, the compiler
won't give you even a warning, and the program may malfunction in
strange ways, which will sometimes be very hard to debug.)

That's what the swap idiom for assignment is all about (in addition to exception
safety).


Cheers, & hth.,

- Alf

 

[Jeff Schwab]

* (e-mail address removed): ....
No. Thinking about that would be premature optimization.

Whether it's slower is a completely separate issue from whether its
speed matters, or whether we "should" be thinking about it yet.

 

[James Kanze]

I've never seen that before, but it does seem like a neat
idea.
It seems like it would be (generally speaking) twice as slow
as manual member-by-member initialization, because of (a)
the temporary object's constructor, plus (b) the assignment
operator.

[/QUOTE]

If it makes a measurable difference, make the functions inline,
and let the compiler's optimizer take care of it.

Just now I managed to get this working using placement new. Is
there anything bad that can happen with the following code
(assuming the constructor does not throw any exceptions):

class B {
public:
B () { }
~B () { }

void Reset () {
this->~B();
new (this) B();
}
};

Well assuming the constructor doesn't throw any exceptions, and
assuming that no one ever derives from the class, and probably
assuming one or two other things, it's OK.

In general, there are just too many assumptions in there for me
to ever want to see it in production code.

I've done a good amount of testing on that just now and it
seems to work fine (even when B has other complex object
members, etc.).

Try adding a virtual member (say a virtual destructor), then
deriving. I think you'll find some mighty strange behavior.
And of course, it's very, very hard to guarantee that a
constructor doesn't throw.

 

[James Kanze]

* Juha Nieminen:

It is again a problem with derived classes, but here "only"
with efficiency and expectations of maintainance programmers
(the latter the most serious).

I don't think it's really a problem with derived classes,
because at this point, they don't exist yet. (Their constructor
hasn't yet been called.) However...

Another problem is that it requires data members to be
default-constructible.

Worse: if some of the data has do nothing constructors, and the
operator= expects it to be initialized. Pointers to dynamic
memory would be a classical case---if the operator= results in
the old values being passed to delete, delete's not going to
like getting passed some random bits.

As you say...

That's what the swap idiom for assignment is all about (in
addition to exception safety).

In simple cases, you may be able to do even simpler than the
swap idiom, but it is the guaranteed general solution (provided
all of the members support it as well).

 

[jason.cipriani]

Check out the swap idiom for implementing assignment operator in terms of copy
constructor.

Is that something like this (from http://www.gotw.ca/gotw/059.htm):

T& T:perator=( const T& other )
{
T temp( other ); // do all the work off to the side
Swap( temp ); // then "commit" the work using
return *this; // nonthrowing operations only
}

And one advantage is there's really not much to do in the = operator,
since you are using the constructors to do a lot the work? So a
Reset() might simply be:

void T::Reset () {
Swap(T());
}

But in that case... I mean, is there a way to implement Swap() that
doesn't involve explicitly typing out code to swap all of the members?
Since you just need to do a "shallow" swap... is something like this
reliable:

void T::Swap (T &other) {
char temp[sizeof(T)];
memcpy(temp, &other, sizeof(T));
memcpy(&other, *this, sizeof(T));
memcpy(*this, temp, sizeof(T));
}

That seems like a horrible way to do it... also, what if "other" is
something derived from T? Is there a way to just use the compilers
default assignment operator even when you've defined your own? Or does
it always make sense to use the "pimpl" idiom when implementing a swap
function, so that you don't have to do something like that?

Jason

 

[Alf P. Steinbach]

* (e-mail address removed):

Is that something like this (from http://www.gotw.ca/gotw/059.htm):

T& T:perator=( const T& other )
{
T temp( other ); // do all the work off to the side
Swap( temp ); // then "commit" the work using
return *this; // nonthrowing operations only
}

Yes, except you can do it neater:

T& operator=( T other )
{
swap( other );
return *this;
}

And one advantage is there's really not much to do in the = operator,
since you are using the constructors to do a lot the work?

Main advantages are exception safety and avoiding redundant code (always a
source of bugs and extra work).

So a
Reset() might simply be:

void T::Reset () {
Swap(T());
}

Well no, it would have to be

void reset() { T().swap( *this ); }

But in that case... I mean, is there a way to implement Swap() that
doesn't involve explicitly typing out code to swap all of the members?
Since you just need to do a "shallow" swap... is something like this
reliable:

void T::Swap (T &other) {
char temp[sizeof(T)];
memcpy(temp, &other, sizeof(T));
memcpy(&other, *this, sizeof(T));
memcpy(*this, temp, sizeof(T));
}

It all depends. Some member might have a pointer back to the object, say.
However, there is a nice little elegant facility for iterating through members
in a little side project of Boost.Spirit, I forget the name (Poenix?), and
possibly that might serve as foundation for a general implementation?

That seems like a horrible way to do it...

memcpy, yes, horrible.

also, what if "other" is something derived from T?

The swap idiom takes care of that, whereas other idioms may not necessarily take
care of that.

Is there a way to just use the compilers
default assignment operator even when you've defined your own?

Depends which assignment operator you have defined. But if you have defined
the copy assignment operator, then no. On the other hand, the compiler will use
your copy assignment operator when it defines one for a derived class.

Or does
it always make sense to use the "pimpl" idiom when implementing a swap
function, so that you don't have to do something like that?

I don't see what the PIMPL idiom has to do with this, but possibly you mean
having a pointer to the real object state so that you only need to swap
pointers. Well that's what std::vector etc. has. And they also have swap
member functions, so, very efficient.


Cheers, & hth.,

- Alf

 

[Alf P. Steinbach]

* James Kanze:

I don't think it's really a problem with derived classes,
because at this point, they don't exist yet. (Their constructor
hasn't yet been called.) However...

Well, I was thinking of the above as a pattern, used also in derived classes.

Then a call chain

Derived:erived
-> Base::Base
-> Base::Base[init-list] // Base members def-inits
-> Base::Base[body]
-> Base:perator=() // Base members assigned
-> Derived:erived[init-list] // Derived members def-inits.
-> Derived:erived[body]
-> Derived:perator=()
-> Base:perator=() // Base members assigned 2nd time.
-> [assignment statements] // Derived members assigned


[snip]

In simple cases, you may be able to do even simpler than the
swap idiom
?

, but it is the guaranteed general solution (provided
all of the members support it as well).

Cheers,

- Alf