Pointer assignment in c++

Question

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?

The compiler cannot implicitly define the assignment- or move- operator if the class contains sub-objects which cannot be assigned (or moved), like const-qualified objects or references. And whatever you try to get around it will cause UB (aside from removing the qualifiers). — Deduplicator
– Deduplicator, Commented Dec 8, 2014 at 5:31
Scale *scale; if (FindSequence(scale)) { would invoke UB if you dereference the pointer, as you do in *oscale = Scale(sx, sy);. (Dereferencing an uninitialized pointer is UB.) You mean this: Scale scale; if (FindSequence(&scale)) {. But what you probably really mean is bool FindSequence(Scale & oscale) and Scale scale; if (FindSequence(scale)) {. — cdhowie
– cdhowie, Commented Dec 8, 2014 at 5:32
Also, declare the SCALE_X and SCALE_Y members non-const. If you have a Scale object then you can change them, but if you have a const Scale object then you can't. Forcing them read-only is bad practice; if the consumer wants a read-only Scale object then they can just declare it const. — cdhowie
– cdhowie, Commented Dec 8, 2014 at 5:34
Got it, do I have to declare a second constructor so that Scale scale; does not throw an error or is there another way around not having to actually create the object? — John
– John, Commented Dec 8, 2014 at 5:38
Why would default constructing a Scale throw an error? Unless it is doing something we don't know about. Maybe you should post the default Scale constructor. — PaulMcKenzie
– PaulMcKenzie, Commented Dec 8, 2014 at 5:41

cdhowie · Accepted Answer · 2014-12-08 05:56:58Z

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.)

Thanks, my main reasoning for the const fields was coming from more of a Java background where final objects didn't make the internals immutables. TIL, thanks!
@John No problem. Note that in C++, methods can be const too, and you cannot call non-const methods on const objects. Inside of const objects, the this pointer points to a const object. This means that you can publish different interfaces depending on whether or not the object being invoked on is const! (For example, standard library containers will return iterator from begin() if the object is not const, but will return const_iterator if the object is const.)
So any method that does not change the state of the object should be const or you won't be able to use it from a const object. (void foo(); versus void foo() const; -- if both exist then which one is selected depends on the const-ness of the object on which it is invoked.)
I see. What is the functional difference between an iterator and const_iterator? Are these separate only because they have to?
@ravi Actually, I usually do the opposite -- I use const iterators unless I need to change the objects in the collection. But that's a topic for a different question.

ravi · Accepted Answer · 2014-12-08 05:37:53Z

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.

Tom · Accepted Answer · 2014-12-08 06:41:24Z

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.

Collectives™ on Stack Overflow

Pointer assignment in c++

3 Answers 3

7 Comments

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

7 Comments

Comments

Comments

Related