CODEWITHSUNDEEP
c++pointers
John
John

Reputation: 816

Pointer assignment in c++

I have a simple struct to hold two const doubles.

struct Scale {
    const double SCALE_X, SCALE_Y;

    Scale(double, double);
};

Scale::Scale(double sx, double sy) :
SCALE_X(sx), SCALE_Y(sy) {
}

I have this function that returns a boolean as confirmation, and sets the passed oscale pointer to whatever calculated value found inside.

bool FindSequence(Scale* oscale) {
    // ...
    *oscale = Scale(sx, sy);
    // ...
    return true;
}

In use:

Scale *scale;
if (FindSequence(scale)) {
    std::cout << "Yes" << std::endl;
}

Compile log:

photon.cpp:286:14: error: use of deleted function 'photon::Scale& photon::Scale::operator=(photon::Scale&&)'
      *oscale = Scale(sx, sy);
              ^
In file included from photon.h:5:0,
                 from photon.cpp:4:
globals.h:76:9: note: 'photon::Scale& photon::Scale::operator=(photon::Scale&&)' is implicitly deleted because the default definition would be ill-formed:
  struct Scale {
         ^
globals.h:76:9: error: non-static const member 'const double photon::Scale::SCALE_X', can't use default assignment operator
globals.h:76:9: error: non-static const member 'const double photon::Scale::SCALE_Y', can't use default assignment operator

Do I have to override the operator=? Is it because the Scale struct is immutable? What am I doing wrong?

Upvotes: 1

Views: 108

Answers (3)

Tom
Tom

Reputation: 7921

I'll add, as an extra note, that if you did really want to have const members and an initialisation function then the way to do it is with placement-new:

bool FindSequence(Scale* oscale) {
    // ...
    oscale = new (oscale) Scale(sx, sy);
    // ...
    return true;
}

This uses your Scale constructor to initialise the pointed-at object, not the assignment operator, avoiding the need for a compiler-generated operator.

As others have pointed out, your code then runs into undefined behaviour because you pass an uninitialised pointer to the FindSequence function. What you probably meant is something like this:

Scale scale(0.0, 0.0);
if (FindSequence(&scale)) {
    std::cout << "Yes" << std::endl;
}

Some would argue that this is a bad idea because it allows you to change a const object after initialisation, though, and I'd expect that your compiler may attempt optimisations on the assumption that those const double members don't change, so you may see strange behaviour if you try this.

Upvotes: 0

cdhowie
cdhowie

Reputation: 168988

There are several problems here. I'll go over them one by one.

const double SCALE_X, SCALE_Y;

This is the primary source of your problem. The compiler won't generate a copy constructor or copy assignment operator if the class has const members; you would have to generate them yourself.

However, I don't think that these members should even be const. If someone has a Scale object then they should be able to change its value, but if they have a const Scale object then they won't be able to, because all (non-mutable) data members "become" const when accessed on a const object.

In other words, let the consumer of the class decide if they want instances to be read-only; non-const Scale objects should be mutable!

With this one change, the compiler will be able to generate sane a copy constructor and a sane copy assignment operator, so that particular error will go away.

However, you do have another error here:

Scale *scale;
if (FindSequence(scale)) {

You are passing in an uninitialized pointer. As soon as FindSequence() dereferences this pointer, undefined behavior is invoked and your program will likely crash.

Typically you would do this using references:

bool FindSequence(Scale & oscale) {

Then the calling code becomes this (just remove the *):

Scale scale;
if (FindSequence(scale)) {

But to do this you will need a default constructor. A sane option would be to have it initialize the members to zero:

Scale::Scale() : SCALE_X(0), SCALE_Y(0) { }

(Alternatively, you could just do Scale scale(0, 0); instead.)

Apply these three changes and the compiler should get past these issues. (It may, of course, run into other issues, but these changes will fix the ones in your question.)

Upvotes: 2

ravi
ravi

Reputation: 10733

There are two things to note here

First compiler wont generate assigment operator for class for which it doesnt know how assignment would be done i.e class containing const and reference members.

Second you are trying to dereference uninitialized pointer which is terrible.

Upvotes: 0

Related Questions