C++ Error "left operand must be l-value"

Question

I am trying to write a C++ program so solve Rubik's cubes. I have defined four classes: Piece, Edge, Corner, and Cube where Corner and Edge are subclasses of Piece.

The Cube class is defined as such:

class Cube{
private:
    Piece* pieces[3][3][3];
public:
    Corner* WRG = new Corner(WHITE, RED, GREEN, WHITE);
    Corner* WGO = new Corner(WHITE, GREEN, ORANGE, WHITE);
    Corner* WOB = new Corner(WHITE, ORANGE, BLUE, WHITE);
    Corner* WBR = new Corner(WHITE, BLUE, RED, WHITE);

    Corner* YRB = new Corner(YELLOW, RED, BLUE, YELLOW);
    Corner* YBO = new Corner(YELLOW, BLUE, ORANGE, YELLOW);
    Corner* YOG = new Corner(YELLOW, ORANGE, GREEN, YELLOW);
    Corner* YGR = new Corner(YELLOW, GREEN, RED, YELLOW);

    Edge* WR = new Edge(WHITE, RED, WHITE);
    Edge* WB = new Edge(WHITE, BLUE, WHITE);
    Edge* WO = new Edge(WHITE, ORANGE, WHITE);
    Edge* WG = new Edge(WHITE, GREEN, WHITE);

    Edge* YR = new Edge(YELLOW, RED, YELLOW);
    Edge* YB = new Edge(YELLOW, BLUE, YELLOW);
    Edge* YO = new Edge(YELLOW, ORANGE, YELLOW);
    Edge* YG = new Edge(YELLOW, GREEN, YELLOW);

    Edge* GO = new Edge(GREEN, ORANGE, GREEN);
    Edge* GR = new Edge(GREEN, RED, GREEN);

    Edge* BO = new Edge(BLUE, ORANGE, BLUE);
    Edge* BR = new Edge(BLUE, RED, BLUE);

    Cube();
    ~Cube();
    void rotateRedClock();
    void rotateRedCounter();
    void rotateOrangeClock();
    void rotateOrangeCounter();
    void rotateYellowClock();
    void rotateYellowCounter();
    void rotateGreenClock();
    void rotateGreenCounter();
    void rotateBlueClock();
    void rotateBlueCounter();
    void rotateWhiteClock();
    void rotateWhiteCounter();

    void doMove(int);
    Piece getPieces();
    Cube* getChildren();
};
Cube::Cube(){
    Piece* pieces[3][3][3] = { { { WRG, WR, WBR }, { GR, NULL, BR }, { YGR, YR, YRB } }, //Red face
                            { { WG, NULL, WB }, { NULL, NULL, NULL }, { YG, NULL, YB } }, //Middle section
                            { { WGO, WO, WOB }, { GO, NULL, BO }, { YOG, YO, YBO } } }; //Orange face
}

This array is stored inside of a Cube object that can shuffle the pointers in the array and change an orientation parameter for each Piece to handle rotations. From what I can tell, this all should work fine.

The problem starts when I try to return an array of Cube objects that contains all the moves possible from the current state.

If I were programming this in Java, it would look like this:

public Cube[] getChildren(){
    Cube children = new Cube[12];
    for (int i = 0; i < 12; i++){
        children[i] = new Cube(this.getPieces()); //Effectively clone this
        children[i].doMove(i); //Does one of the 12 available moves on the cube
    }
    return children;
}

In C++, however, I can't seem to accomplish this goal. I tried the following:

Cube* Cube::getChildren(){
    Cube* children = new Cube[12];
    for (int i = 0; i < 12; i++){
        children[i] = Cube();
        children[i].pieces = pieces;
        children[i].doMove(i);
    }
    return children;
}

But I get an error on this line:

children[i].pieces = pieces;

It says:

error C2106: '=' : left operand must be l-value

I am new to C++, and this error is probably a result of my lack of understanding of certain concepts. I would like to know what I am doing wrong so I can avoid this sort of problem in the future. Thanks in advance!

Answer 1

Don't use raw pointers and don't use new anywhere in your code. The C++ equivalent of Java's Cube[] is std::vector<Cube>. Your sample function could look like:

std::vector<Cube> Cube::getChildren() const
{
    // 12 copies of current state
    std::vector<Cube> children(12, *this);

    for (int i = 0; i < children.size(); i++)
    {
        children[i].doMove(i);
    }

    return children;
}

There are other changes to be made before this will work though (as things stand, a huge amount of memory will be leaked, and the "copies" will affect each other).

---

I'm guessing that the last argument to your Corner and Edge constructor is meant to be some sort of orientation indicator, which you will change when the pieces are rotating. So the variables WRG, WGO are supposed to be mutable and encode the current orientation of that piece.

In C++, objects should have value semantics. In this case, it means that copying the object should do a "deep copy", aka. a clone. There should not ever be code for implementing the copy except for inside the object's constructor.

So the issue of attempting children[i].pieces = pieces inside the getChildren function would never even come up if your objects are designed to use value semantics.

If your object design includes 27 pointers to mutable members of the class, then the default-generated copy constructor will do the wrong thing because:

• all "copies" actually have the same pointer to pieces -- there's only one actual set of pieces. (Reorienting the piece in a new cube will reorient the piece in the cube it was copied from)
• Even if that is fixed, the "copy" will point to pieces of the original cube, instead of pieces of the copied cube.

So, this is not a good design in C++.

It would be better to just hold the pieces by value. An improvement on your original code (but still not workable yet) would be:

Corner WRG {WHITE, RED, GREEN, WHITE};
Edge WR {WHITE, RED, WHITE};

Pieces *pieces[3][3][3] =
{ {&WRG, &WR, &WBR}, {&GR, nullptr, &BR}, // etc...

With this version, at least there is no memory leak, however there is still the problem that the default-generated copy constructor will have the new cube's Piece pointers copying at the old cube's Pieces.

---

There are three ways to fix this:

1. Write a copy-constructor (and a copy-assignment operator) which detects which piece the old cube's pointers point to and make each corresponding pointer in the new cube point to the new piece
2. Make the pieces be static so there really is only one set of pieces. The orientation will be remembered using separate variables.
3. Hold the pieces by value instead of by pointer.

1 is what you are trying to do at the moment and actually harder than it looks; even if you fix the compilation error by using std::copy or equivalent, you still have the pointers pointing to the old cube's pieces.

2 is a good idea. If there is only one set of pieces then you can copy around pointers to pieces without causing any trouble. Of course, then you need each Cube to have a new array to represent the current orientation of each piece . That's still simpler than #1 though!

This option also has the big bonus of decimating the memory footprint of each state.

(see below for more comments on #3 and memory usage).

---

Here is how an implementation of strategy 2 might look like. In the definitions of Edge and Corner take out the field corresponding to the orientation.

class Cube
{
    // "static" means only one copy of each for the whole program
    // The constructor arguments for each one are placed in the .cpp file
    static constexpr Corner WRG, WGO, WOB, /*....*/ ;
    static constexpr Edge WR, GR, /*.....*/ ;

    Pieces const *pieces[3][3][3] =
    { {&WRG, &WR, &WBR}, {&GR, nullptr, &BR}, // etc...

    typedef unsigned char Orientation;

    Orientation orientation[3][3][3] = { };

public:
    // no default constructor needed if you got the above lists right
    // no destructor needed either way
    // Cube();  

    void rotateRedClock();
    void rotateRedCounter();
    // etc. - you'll probably find it easier to roll all of these into
    // a single function that takes the face and the direction as parameter

    void doMove(int);    // suggest using an enum to describe possible moves

    // not necessary getPieces();

    vector<Cube> getChildren() const;
};

---

If you are planning some sort of solving algorithm, you'll want to reduce the memory footprint of each state. So 3 is also a good idea.

If you adopt #2, then doing #3 is not quite so pressing - you can make your code work via approach #2 and optimize it later.

To use #3, you will need to discard the idea of using Piece * polymorphically.

However you don't need this feature anyway because you can tell from the array indices whether the piece is supposed to be a Corner or an Edge. I would suggest just using Piece to be basically what Corner is now but without the orientation; and ignore the third field when you're expecting an edge.

My suggestion would be to replace the table of 27 pointers with a table of 27 bytes . Then, in your .cpp file you'd have a lookup table that you can use to get the piece corresponding to that index.

Check the edit history for my post to see a rough outline of how that might look. (I initially wrote that, but then decided that the current version would be a bit easier to digest!)

Answer 2

You didn't provide how Cube::pieces look like pieces and have type int[3][3][3]. The problem that it is C-style array and C very weakly typed, so neither C nor C++ cannot distinguish int[3][3][3] from int[2][2] - type information is lost.

Consider using C++ array types - they define copy constructors and assignment operators and save their sizes internally, so they will do all work for you. Lets take a ride!

C style array

As we already know, this will not work, this is just an example.

int main() {
    int i[2][2] = { {0, -1}, {1, 2} };
    int b[2][2];    
    b = i; /* cube.cpp:5:9: error: invalid array assignment */
}

std::vector

For that you will need to define vector of vectors of vectors:

#include <vector>

int main() {
    std::vector<std::vector<int> > i = { {0, -1}, {1, 2} };
    std::vector<std::vector<int> > b;    
    b = i;
}

boost::multi_array

(This will require external library)

#include <boost/multi_array.hpp>

int main() {
    boost::multi_array<int, 2> i(boost::extents[2][2]); 
    /* Note that for multi_array we need single-dimentional initializer */
    auto _i = { /*{*/ 0, -1 /*}*/, 
                /*{*/ 1, 2  /*}*/ };    
    i.assign(_i.begin(), _i.end());

    boost::multi_array<int, 2> b(boost::extents[2][2]);    
    b = i;
}

This may seem more complex than other solutions, but multi_array may be a bit more efficient than vector of vectors.

Answer 3

In C++, you cannot assign another array to an array of fixed length. You have to copy the values individually. (Not 100% sure, but pretty sure.)

In the definition of class Cube, did you mean a pointer to such an array, i.e. did you mean

typedef Piece PieceArray [3][3][3];
PieceArray * pieces;

In your code, you do not declare a pointer to an array of pieces, but an array of pointers to pieces.

Do yourself a favour and use std::vector.