0

In the following example vec keeps the same order before and after calling std::reverse. If I make Foo.a not const and remove the copy assignment operator then the vector gets reversed properly, however I don't understand why.

When debugging the assignment operator does seem to copy the object properly when called inside std::reverse so I don't understand what I should be doing differently.

#include <vector>
#include <algorithm>
#include <iostream>

struct Foo
{
    Foo(int a, int b) : a(a), b(b) {}
    Foo operator=(const Foo& other)
    {
        return Foo(other);
    }
    const int a;
    int b;
    Foo doThing()
    {
        Foo copy(*this);
        copy.b++;
        return copy;
    }
};

void print(std::vector<Foo>& vec)
{
    for (const auto& foo : vec)
    {
        std::cout << foo.b << '\n';
    }
}

int main()
{
    Foo initial(-1, 0);
    std::vector<Foo> vec;
    vec.push_back(initial);
    for (size_t i = 0; i < 10; i++)
    {
        vec.push_back(vec[i].doThing());
    }
    std::cout << "before std::reverse \n\n";
    print(vec);
    std::reverse(vec.begin(), vec.end());
    std::cout << "\nafter std::reverse \n\n";
    print(vec);
    std::cout.flush();
}
Janilson
  • 1,042
  • 1
  • 9
  • 23
  • 4
    The `operator=` overload is completely broken. It makes absolutely no changes to the object being assigned to. And, yes, with a `const` class member no meaningful assignment operator can be implemented in C++. – Sam Varshavchik Jan 15 '23 at 23:06
  • 4
    That `operator=` doesn’t modify the left-hand object (which is accessed through `this`). – Pete Becker Jan 15 '23 at 23:06
  • is the return value of the operator not assigned to the left-hand object? could you give me an example of how the operator should be implemented in a way that std::reverse works? – Janilson Jan 15 '23 at 23:15
  • [What are the basic rules and idioms for operator overloading?](https://stackoverflow.com/questions/4421706/what-are-the-basic-rules-and-idioms-for-operator-overloading) – Igor Tandetnik Jan 15 '23 at 23:25
  • 1
    "is the return value of the operator not assigned to the left-hand object?" Assigned how - by an assignment operator? Is it turtles all the way down? – Igor Tandetnik Jan 15 '23 at 23:29
  • The compiler could have some special rule for that operator, idk. If the return value is simply thrown away that's very unintuitive. Even if it's obvious for you there's no need to be an ass about it, if everyone knew everything this site would be quite pointless. Also even though the link you provided does say a lot about C++ operators it doesn't seem to help me at all with my original problem which is how I get std::reverse to work in an object like the one I provided in the example. – Janilson Jan 15 '23 at 23:34
  • @SamVarshavchik "And, yes, with a const class member no meaningful assignment operator can be implemented in C++." True in the past but It can as of c++20. – doug Jan 15 '23 at 23:44
  • *"std::reverse doesn't work"* -- Before jumping to this conclusion, you should have tested it more. Testing with the default copy assignment was good, as that tested `std::reverse` in isolation. The next test should have been of your copy assignment in isolation, something like `int main() { Foo initial(-1, 0); Foo final(1, 2); std::cout << "Before: " << final.b << "\n"; final = initiall; std::cout << "After: " << final.b << "\n"; }` – JaMiT Jan 15 '23 at 23:58
  • Whether or not the return value of `operator=` is "simply thrown away" depends on the calling code. If you write something like `x = 7;`, sure, you're throwing it away. But if you return a reference to `*this`, you could write something like `x = y = z = 7;` where the return value of `z.operator=(7)` is passed as an argument to `y.operator=`. – Nathan Pierson Jan 16 '23 at 00:36

1 Answers1

1

This wasn't possible prior to c++20 as const members could not be changed via assignment w/o UB and is the reason const members in objects were/are strongly discouraged. But now it is. But you still have to write an assignment operator. To future proof the code (past c++23) you should also include a copy ctor as the implicit copy ctor may be removed. Here's the working code and involves no UB:

#include <vector>
#include <algorithm>
#include <iostream>
#include <memory>

struct Foo
{
    Foo(int a, int b) : a(a), b(b) {}
    Foo& operator=(const Foo& other)
    {
        if (this == &other)
            return *this;
        std::destroy_at(this);
        std::construct_at(this, other);
        return *this;
    }
    Foo(const Foo& a) = default;  // Future proof
    const int a;
    int b;
    Foo doThing()
    {
        Foo copy(*this);
        copy.b++;
        return copy;
    }
};

void print(std::vector<Foo>& vec)
{
    for (const auto& foo : vec)
    {
        std::cout << foo.b << '\n';
    }
}

int main()
{
    Foo initial(-1, 0);
    std::vector<Foo> vec;
    vec.push_back(initial);
    for (size_t i = 0; i < 10; i++)
    {
        vec.push_back(vec[i].doThing());
    }
    std::cout << "before std::reverse \n\n";
    print(vec);
    std::reverse(vec.begin(), vec.end());
    std::cout << "\nafter std::reverse \n\n";
    print(vec);
    std::cout.flush();
}
doug
  • 3,840
  • 1
  • 14
  • 18