CODEWITHSUNDEEP
c++initializationthis
KKOrange
KKOrange

Reputation: 590

Are there any consequences from using *this to initialise a class?

In a small game I'm writing, I have a class Weapon with two constructors, one which takes in some parameters to produce a custom weapon, and one that grabs a default one (the CHAIN_GUN):

Weapon::Weapon (void) {
    // Standard weapon
    *this = getWeapon(CHAIN_GUN);
    return;
}

Question: Are there any negative consequences from using *this and operator= to initialise a class?

Upvotes: 30

Views: 1519

Answers (5)

Filip Roséen
Filip Roséen

Reputation: 63797

Imagine that someone asked you to draw a painting... would you;

  • first draw your default (1st) (that familiar smilie face you like so much),
  • then draw what that someone asked for (2nd),
  • only to draw the same thing one more time, but on the canvas containing your default,
  • and then burn the 2nd painting?

This post will try to explain why this simile is relevant.

WHY IS IT A BAD IDEA?

I've never seen a default-constructor implemented with the use of the assignment-operator, and honestly it's not something that I'd recommend, nor support during a code review.

The major problem with such code is that we are, by definition, constructing two objects (instead of one) and calling a member-function, meaning that we construct all our members two times, and later having to copy/move initialize all members by calling the assignment operator.

It's unintuitive that upon requesting construction of 1 object, we construct 2 objects, only to later copy the values from the 2nd to the 1st and discard the 2nd.

Conclusion: Don't do it.

( Note: In a case where Weapon has base-classes it will be even worse )

( Note: Another potential danger is that the factory function accidentially uses the default-constructor, resulting in an infinite recursion not caught during compilation, as noted by @Ratchet Freat )

PROPOSED SOLUTION

In your specific case you are far better off using a default-argument in your constructor, as in the below example.

class Weapon {
public:
  Weapon(WeaponType w_type = CHAIN_GUN);
  ...
}


Weapon w1;             // w_type = CHAIN_GUN
Weapon w2 (KNOWLEDGE); // the most powerful weapon

( Note: An alternative to the above would be to use a delegating constructor, available in C++11 )

Upvotes: 33

Arkady
Arkady

Reputation: 2207

I'm sure yes.

You already created object inside your "getWeapon" function, and then you copying it, that may be long operation. So, at least you have to try move semantic.

But. If inside "getWeapon" you call to constructor (and you do, somehow "getWeapon" have to create your class to return it to your copy operation), you create very unclear architecture, when one constructor calls function that calls another constructor.

I believe you have to separate your parameters initialization to private functions, that have to be called from your constructors the way you want to.

Upvotes: 3

Tony Delroy
Tony Delroy

Reputation: 106106

One thing to keep in mind is that if operator= - or any function it calls - is virtual, the derived-class version won't be invoked. This could result in uninitialised fields and later Undefined Behaviour, but it all depends on your data members.

More generally, your bases and data members are guaranteed to have been initialised if they have constructors or appear in the initialiser list (or with C++11 are assigned to in the class declaration) - so apart from the virtual issue above, operator= will often work without Undefined Behaviour.

If a base or member has been initialised before operator=() is invoked, then the initial value is overwritten before it's used anyway, the optimiser may or may not be able to remove the first initialisation. For example:

std::string s_;
Q* p_;
int i_;

X(const X& rhs)
  : p_(nullptr)  // have to initialise as operator= will delete
{
    // s_ is default initialised then assigned - optimiser may help
    // i_ not initialised but if operator= sets without reading, all's good
    *this = rhs;
}

As you can see, it's a bit error prone, and even if you get it right someone coming along later to update operator= might not check for a constructor (ab)using it....

You could end up with infinite recursion leading to stack overflow if getWeapon() uses the Prototype or Flyweight Patterns and tries to copy-construct the Weapon it returns.

Taking a step back, there's the question of why getWeapon(CHAIN_GUN); exists in that form. If we need a function that creates a weapon based on a weapon type, on the face of it a Weapon(Weapon_Type); constructor seems a reasonable option. That said, there are rare but plentiful edge cases where getWeapon might return something other than a Weapon object that can never-the-less be assigned to a Weapon, or might be kept separate for build/deployment reasons....

Upvotes: 7

Vaughn Cato
Vaughn Cato

Reputation: 64308

Using the assignment operator to implement a constructor is rarely a good idea. In your case, for example, you could just use a default parameter:

Weapon::Weapon(GunType g = CHAIN_GUN)
: // Initialize members based on g
{
}

In other cases, you might use a delegating constructor (with C++11 or later):

Weapon::Weapon(GunType g)
: // Initialize members based on g
{
}

Weapon::Weapon()
: Weapon(CHAIN_GUN) // Delegate to other constructor
{
}

Upvotes: 16

Remy Lebeau
Remy Lebeau

Reputation: 596352

If you have defined a non-copy = assignment operator that lets the Weapon change its type after construction, then implementing the constructor in terms of assignment works just fine, and is a good way to centralize your initialization code. But if a Weapon is not meant to change type after construction, then a non-copy = assignment operator does not make much sense to have, let alone use for initialization.

Upvotes: 3

Related Questions