c++ pointer arguments: does it make sense to create a copy of the value pointed too, then store the copy as pointer again?

Question

I'm migrating a project to c++ because I hit a performance ceiling while developing it in c#. It's my first time using c++, however, and I'm finding myself doing something a lot that doesn't seem quite right...

Consider the following abstracted example:

class ClassC
{
    ClassC::ClassC(int option)
    {
        //do something
    }
}

class ClassB
{
    ClassC* objC

    ClassB::ClassB(ClassC* objC)
    {
        this->objC = new ClassC(*objC);
    }
}

class ClassA
{
    void functionA(void)
    {
        ClassB objB (&ClassC(2));
    }
}

ClassA has a function which creates a ClassB. ClassB's constructor accepts a ClassC, objC. objC is passed by reference because ClassC is not a primitive type, but its stored by reference because ClassC has no default constructor. However, because objC is created in static memory and will be destructed when functionA completes, ClassB needs to copy to the value pointed by objC into dynamic memory, then store a pointer to that copy.

This seems very round-a-bout to me, and makes me feel like im approaching something incorrectly. Is this a standard thing to do in c++?

EDIT: Everyone seems to be saying that the line ClassB objB (&ClassC(2)); is incorrect because the value of the ClassC object will be lost before ClassB can make a copy of it. But I have compiled my example and this is not the case. Here is revised, working code:

class ClassC
{
    int option;

public:
    ClassC::ClassC(int option)
    {
        this->option = option;
    }

    int ClassC::getOption(void)
    {
        return option;
    }
};

class ClassB
{
    ClassC* objC;

public:
    ClassB::ClassB(ClassC* objC)
    {
        this->objC = new ClassC(*objC);
    }

    int ClassB::getOption(void)
    {
        return objC->getOption();
    }
};

class ClassA
{
public:
    static ClassB functionA(void)
    {
        return ClassB (&ClassC(2));
    }
};

int main(void)
{
    ClassB objB = ClassA::functionA();

    int test = objB.getOption(); //test = 2, therefore objC was copied successfully.

    return 0;
}

Answer 1

Not sure what's your real question, but your code seems fragile. I'd like to rewrite your code as you have shown to this way:

class ClassC
{
    explicit ClassC(int option)
//  ^^^^^^^^ stop implicit conversion, if constructor takes one parameter
    {
        //do something
    }
};

class ClassB
{
    ClassC objC;                         // store by value instead of pointer. 
                                         // Even smart pointer will be better option than raw pointer

    explicit ClassB(const ClassC& objC)  // pass by const reference instead
    : objC(objC)                         // use member initializer list to initialize members
    {
    }
};

class ClassA
{
    void functionA(void)
    {
        ClassB objB(ClassC(2));
    }
};

Answer 2

Taking the address of a temporary and saving it off for later use is a big no-no.

ClassB objB (&ClassC(2));  // taking address of temporary

Further, even passing a const-reference of a temporary across a function parameter list will not extend the lifetime further than the function invoke. I.e. once the constructor is done firing the reference is toast, so this:

class ClassB
{
    const ClassC& objC;

public:
    ClassB(const ClassC& objC) : objC(objC)
    {
    }
};

won't work either. More info can be read here about the details for why.

It would work if you did this:

ClassC objC;
ClassB objB(objC);

but then again, so would your original sample.

One way to have the lifetime of an external object guaranteed is to dynamically allocate the object through smart-pointer ownership. Consider this:

class ClassB
{
    std::shared_ptr<ClassC> ptrC;

public:

    ClassB(std::shared_ptr<ClassC> ptrC)
        : ptrC(ptrC)
    {
        // access the instance with ptrC->member()
    }
};

Now you can do this:

ClassB objB(std::make_shared<ClassC>(2));

and even if objB is value-copied (like in a sort-operation on a container, etc) the shared instance is still intact. The last man out the door turns off the lights (in this case, deletes the shared ClassC object).

Obviously its rather pointless for a single instance that will only be held by a single parent. In that case, I totally concur with other answers that strongly suggest you use move-semantics. If, however, you have a true need for a shared resource this is one way to consider doing it.

---

EDIT Adding pass-through constructor to ClassB as a trivial example.

I just realized everyone was so harped on assisting you in constructing your ClassC object, that maybe all you need is a way to provide parameters to objC for construction. I.e. Perhaps you fully intend on objB to outright own its own private instance of objC and all you need is a way to get parameters to it for initialization.

This is what a constructor initializer list is made for. See the code below, which (based on your comments, will probably work for you and is much simpler to understand.

class ClassB
{
    ClassC objC;

public:
    // default constructor. initializes objC with default value
    ClassB() : objC(0)
    {
    }

    // explicit pass-through of params to `objC` construction
    explicit ClassB(int option) : objC(option)
    {
    }
};

This makes your code in ClassA simply this:

ClassB objB(2);

This will invoke ClassB::ClassB(int), passing the provided parameter to construction of the internal objC object instance of type ClassC.

Answer 3

Your constructor for ClassC is irrelevant as what will be called is the copy-constructor

class ClassC
{
    ClassC(int option) // defines a constructor that takes an int
    {
        //do something
    }
}

class ClassB
{
    ClassC* objC

    ClassB(ClassC* objC)
    {
        this->objC = new ClassC(*objC); // dereferences objC calling ClassC::ClassC(const Class& obj) - the default copy constructor.
    }
}

class ClassA
{
    void functionA(void)
    {
        ClassB objB (&ClassC(2)); // passing a reference to a temporary ... bad idea, but since it is copied in ClassB (the object, not the pointer), it will appear okay - if your compiler lets this compile (newer ones should/will likely throw an error "cannot take address of rvalue temporary")
    }
}

All in all, this code would be better off with many of the suggestions already mentioned, but it was worth noting that the copy-constructor for ClassC is what was called in ClassB.

Answer 4

ClassB objB (&ClassC(2));

This is a bad thing to do. You are getting the address of a temporary object, which will vanish after this line. As already stated, try to use references instead of pointers. Most of the time, you can replace a pointer with a reference, which is a very safe approach (it will never be "NULL").

You could write

class ClassB
{
    ClassC& objC;

    ClassB::ClassB(const ClassC& objC) :
        objC(objC)
    {
    }
}