const member and assignment operator. How to avoid the undefined behavior?

Question

I answered the question about std::vector of objects and const-correctness, and received a comment about undefined behavior. I do not agree and therefore I have a question.

Consider the class with const member:

class A { 
public: 
    const int c; // must not be modified! 
    A(int c) : c(c) {} 
    A(const A& copy) : c(copy.c) { }     
    // No assignment operator
}; 

I want to have an assignment operator but I do not want to use const_cast like in the following code from one of the answers:

A& operator=(const A& assign) 
{ 
    *const_cast<int*> (&c)= assign.c;  // very very bad, IMHO, it is undefined behavior
    return *this; 
} 

My solution is

// Custom-defined assignment operator
A& operator=(const A& right)  
{  
    if (this == &right) return *this;  

    // manually call the destructor of the old left-side object
    // (`this`) in the assignment operation to clean it up
    this->~A(); 
    // use "placement new" syntax to copy-construct a new `A` 
    // object from `right` into left (at address `this`)
    new (this) A(right); 
    return *this;  
}  

Do I have undefined behavior (UB)?

What would be a solution without UB?

Solution 1

Your code causes undefined behavior.

Not just "undefined if A is used as a base class and this, that or the other". Actually undefined, always. return *this is already UB, because this is not guaranteed to refer to the new object.

Specifically, consider 3.8/7:

If, after the lifetime of an object has ended and before the storage which the object occupied is reused or released, a new object is created at the storage location which the original object occupied, a pointer that pointed to the original object, a reference that referred to the original object, or the name of the original object will automatically refer to the new object and, once the lifetime of the new object has started, can be used to manipulate the new object, if:

...

— the type of the original object is not const-qualified, and, if a class type, does not contain any non-static data member whose type is const-qualified or a reference type,

Now, "after the lifetime of an object has ended and before the storage which the object occupied is reused or released, a new object is created at the storage location which the original object occupied" is exactly what you are doing.

Your object is of class type, and it does contain a non-static data member whose type is const-qualified. Therefore, after your assignment operator has run, pointers, references and names referring to the old object are not guaranteed to refer to the new object and to be usable to manipulate it.

As a concrete example of what might go wrong, consider:

A x(1);
B y(2);
std::cout << x.c << "\n";
x = y;
std::cout << x.c << "\n";

Expect this output?

1
2

Wrong! It's plausible you might get that output, but the reason const members are an exception to the rule stated in 3.8/7, is so that the compiler can treat x.c as the const object that it claims to be. In other words, the compiler is allowed to treat this code as if it was:

A x(1);
B y(2);
int tmp = x.c
std::cout << tmp << "\n";
x = y;
std::cout << tmp << "\n";

Because (informally) const objects do not change their values. The potential value of this guarantee when optimizing code involving const objects should be obvious. For there to be any way to modify x.c without invoking UB, this guarantee would have to be removed. So, as long as the standard writers have done their job without errors, there is no way to do what you want.

[*] In fact I have my doubts about using this as the argument to placement new - possibly you should have copied it to a void* first, and used that. But I'm not bothered whether that specifically is UB, since it wouldn't save the function as a whole.

Solution 2

First: When you make a data member const, you're telling the compiler and all the world that this data member never changes. Of course then you cannot assign to it and you certainly must not trick the compiler into accepting code that does so, no matter how clever the trick.
You can either have a const data member or an assignment operator assigning to all data members. You can't have both.

As for your "solution" to the problem:
I suppose that calling the destructor on an object within a member function invoked for that objects would invoke UB right away. Invoking a constructor on uninitialized raw data to create an object from within a member function that's been invoked for an object that resided where now the constructor is invoked on raw data... also very much sounds like UB to me. (Hell, just spelling this out makes my toenails curl.) And, no, I don't have chapter and verse of the standard for that. I hate reading the standard. I think I can't stand its meter.

However, technicalities aside, I admit that you might get away with your "solution" on just about every platform as long as the code stays as simple as in your example. Still, this doesn't make it a good solution. In fact, I'd argue it's not even an acceptable solution, because IME code never stays as simple as that. Over the years it will get extended, changed, mutated, and twisted and then it will silently fail and require a mind-numbing 36hrs shift of debugging in order to find the problem. I don't know about you, but whenever I find a piece of code like this responsible for 36hrs of debugging fun I want to strangle the miserable dumb-wit who did this to me.

Herb Sutter, in his GotW #23, dissects this idea piece by piece and finally concludes that it "is full of pitfalls, it's often wrong, and it makes life a living hell for the authors of derived classes... never use the trick of implementing copy assignment in terms of copy construction by using an explicit destructor followed by placement new, even though this trick crops up every three months on the newsgroups" (emphasize mine).

Solution 3

How can you possibly assign to an A if it has a const member? You're trying to accomplish something that's fundamentally impossible. Your solution has no new behaviour over the original, which is not necessarily UB but yours most definitely is.

The simple fact is, you're changing a const member. You either need to un-const your member, or ditch the assignment operator. There is no solution to your problem- it's a total contradiction.

Edit for more clarity:

Const cast does not always introduce undefined behaviour. You, however, most certainly did. Apart from anything else, it is undefined not to call all destructors- and you didn't even call the right one- before you placed into it unless you knew for certain that T is a POD class. In addition, there's owch-time undefined behaviours involved with various forms of inheritance.

You do invoke undefined behaviour, and you can avoid this by not trying to assign to a const object.

Solution 4

According to the newer C++ standard draft version N4861 it seems to be no longer undefined behaviour (link):

If, after the lifetime of an object has ended and before the storage which the object occupied is reused or released, a new object is created at the storage location which the original object occupied, a pointer that pointed to the original object, a reference that referred to the original object, or the name of the original object will automatically refer to the new object and, once the lifetime of the new object has started, can be used to manipulate the new object, if the original object is transparently replaceable (see below) by the new object. An object o1 is transparently replaceable by an object o2 if:

• the storage that o2 occupies exactly overlays the storage that o1 occupied, and
• o1 and o2 are of the same type (ignoring the top-level cv-qualifiers), and
• o1 is not a complete const object, and
• neither o1 nor o2 is a potentially-overlapping subobject ([intro.object]), and
• either o1 and o2 are both complete objects, or o1 and o2 are direct subobjects of objects p1 and p2, respectively, and p1 is transparently replaceable by p2.

Here you can find only "o1 is not a complete const object" regarding const, which is true in this case. But of course you have to ensure that all other conditions are not violated, too.

Solution 5

If you definitely want to have an immutable (but assignable) member, then without UB you can lay things out like this:

#include <iostream>

class ConstC
{
    int c;
protected:
    ConstC(int n): c(n) {}
    int get() const { return c; }
};

class A: private ConstC
{
public:
    A(int n): ConstC(n) {}
    friend std::ostream& operator<< (std::ostream& os, const A& a)
    {
        return os << a.get();
    }
};

int main()
{
    A first(10);
    A second(20);
    std::cout << first << ' ' << second << '\n';
    first = second;
    std::cout << first << ' ' << second << '\n';
}

View more solutions

Comments

[Alexey Malistov]

I want vector<A>::push_back(a) to work. It is clear that assignment operator must replace all member data with new data.

[sbi]

@Alexey: <shrug> You might just as well want to have US$10 million right now, "no arguments". You still won't get it.

[Puppy]

@Alexey: Nobody cares what you want. You can't shout at logic, the Standard, or your compiler and make it work. The fact is that you can't have what you want. End of story.

[Jonathan Grynspan]

And I want free cake every day for lunch, but it ain't gonna happen. What you want is fundamentally incompatible with C++. Perhaps you should take a step back--clearly, the class you're creating is not itself const, because instances can be modified, and the c field is not const because modifying the instance modifies c. c should, therefore, not be marked const. Make it non-const and make it private, and add a member function int getFoo() const that returns the value, rather than trying to jump through hoops to do what C++ and basic logic say is a nonsensical thing.

[Alexey Malistov]

@sbi and @DeadMG. My questions are Do I have undefined behavior? and How to avoid UB?. Where is your answer?

[Alexey Malistov]

My questions are Do I have undefined behavior? and How to avoid UB?. Where is your answer?

[Jonathan Grynspan]

I should say that there's nothing wrong with asking your original question, Alexey. const can confuse even veteran programmers sometimes. But "I want to have both" is a meaningless follow-up comment.

[Mark B]

@Alexey It's not clear why you want to change something you've explicitly told the compiler will never change.

[Roddy]

+1 for the GotW link. I think that by itself says why your "solution" was worthy of a downvote.

[Mark B]

Your first example is in fact UB because c is an actual const item.

[Puppy]

@Alexey: You do have undefined behaviour in the bucketloads, and you can avoid it by not trying to assign to a const object.

[André Caron]

Wouldn't it only be UB if the A instance was found in some read-only storage location?

[André Caron]

@Alexey: you should understand that, regardless of technical limitations, what you are asking for just doesn't make sense. Take the const qualifier as a declaration of intent that c will never change. Then, you want to change that value. How does that not sound plain wrong to you?

[Cheers and hth. - Alf]

é: no, always UB. See Steve Jessop's answer for ref.

[sbi]

@Alexey: There, I added a paragraph telling you what I think is UB about it.

[Steve Jessop]

@sbi: "I don't have chapter and verse of the standard for that" - Actually, I think it would be defined behaviour if not for the const data member. Perhaps poor design, for all the reasons Herb Sutter and others have raised, but AFAIK defined, as long as it's used only on objects whose dynamic type is A. This is based on the chapter and verse from my answer.

[sbi]

@Steve: Yeah, I had already up-voted your answer for that. I know I should bite the bullet and start to learn to find my way around in the standard. But now that I'm payed to write C#, I have even less reason to do so...

[Prasoon Saurav]

Excellent find. Better than @sbi's answer I think. +1 :)

[Indiana Kernick]

Could std::launder be used to avoid UB?

[supercat]

If the Standard is going to allow a structure to have const members, what possible sensible meaning could that have other than "This object will only be changed by overwriting the parent structure, an action which may cause the values of any existing pointers or references to members of that object to become indeterminate"? The authors of the Standards didn't think compiler writers needed to be spoon-fed every detail of how they should process each and every corner case in situations where one action would be useful and nothing else would make sense.

[Gabriel Staples]

This much more recent answer by @Bernd seems to be the correct answer today: stackoverflow.com/a/63489092/4561887.

[doug]

It's no longer UB in c++20. Objects that contain consts, but are not themselves consts, can be overlaid in the memory of the same type w/o UB.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1971r0.html‌​#RU007 I like the Gotw#23