0


The Problem: When I attempt to assign an IntArray object by index I get the following error:

"Expression is not assignable."

The error is produced by the following line of code in iadrv.cpp:

IntArray a(10);
for(int i = a.low(); i <= a.high(); i++)
    a[i] = i * 10;

I am able to assign an entire IntArray object to another like so, a = b;, however when a specific index is referred to the "expression is not assignable" error occurs.

EDIT: I removed the const declaration from most of the functions and I am not getting the "Expression is not assignable" error anymore. However, the setName now gives the error:

"ISO C++ 11 does not allow conversion from string literal to 'char *'"

This error is cause by the following code in iadrv.cpp:

a.setName("a");


Program Explanation:

I have written a class IntArray (in C++) in which the following operators are overloaded:

  • "[ ]" : allows index range checking
  • "=" : allows array assignment
  • "+" : allows the sum of two arrays to be assigned to a third array
  • "+=" : allows the sum of two arrays to be assigned to the first array
  • "<<" : allows the contents of an array to be output

The program also includes functions:

  • setName : sets the name of the IntArray object
  • getName : returns the name of the IntArray object
  • low : returns the smallest legal index
  • high : returns the largest legal index
  • length : returns the number of elements

A driver program (iadrv.cpp, iadrv.h) will run tests on the IntArray class (IntArray.cpp, IntArray.h) to determine if all operators were properly overloaded.

Note: that for each array test data, the driver will simply multiply the array index by 10 immediately after each array is initialized or modified and output its contents. When the program encounters a run-time error, it should "simulate"a halt with appropriate diagnostics rather than actually halting the program.


The Code:

IntArray.h

//  IntArray.h

#ifndef __IntArray__IntArray__
#define __IntArray__IntArray__

#include <iostream>
#include <fstream>
#include <iomanip>

using namespace std;

class IntArray {
private:
    int a, b;
    int size;
    int * num;
    char * name;
public:
    IntArray(int start, int finish);
    IntArray(int finish = 10);
    IntArray(const IntArray &); //constructor copy
    ~IntArray();
    int low() const;
    int high() const;
    char * getName() const;
    //removed the const declaration from functions below
    int & operator [] (int);     //made to return int&
    friend ostream & operator << (ostream &, IntArray &);
    void setName(char *);
    int length() const;
    const IntArray & operator = (IntArray &);
    const IntArray & operator + (IntArray &);
    bool operator += (IntArray &);

};

#endif /* defined(__IntArray__IntArray__) */

IntArray.cpp

//  IntArray.cpp

#include "IntArray.h"

#include <iostream>
#include <fstream>

using namespace std;

extern ofstream csis;

IntArray::IntArray(int start, int finish) {
    if (start > finish) {
        cout << "Simulating a halt.";
        a = finish;
        b = start;
    }
    else {
        a = start;
        b = finish;
    }
    size = b-a;
    num = new int[size];
    name = new char[1];
    for (int i = 0; i < size; i++) {
        num[i] = 0;
    }
}
IntArray::IntArray(int finish) {
    size = finish;
    a = 0;
    b = finish - 1;
    num = new int[size];
    name = new char[1];
    for (int i = 0; i < size; i++) {
        num[i] = 0;
    }
}
IntArray::IntArray (const IntArray & right): size(right.size) {
    num = new int[size];
    name = new char[1];
    for (int i = 0; i < size; i++) {
        num[i] = right.num[i];
    }
}
IntArray::~IntArray() {
    delete[] num;
    delete [] name;
}
int IntArray::low() const{
    return a;
}
int IntArray::high() const{
    return b;
}
char * IntArray::getName() const{
    return name;
}
void IntArray::setName(char * n) {
    name = n;
}
//removed const declarations
//made to return int&
int & IntArray::operator [] (int subscript) const{
    if (subscript < a || subscript > b) {
        cout << "subscript: " << subscript << endl;
        cout << "Out of bound error. Simulating a halt." << endl;
        return num[a];
    }
    return num[subscript-a];
}
int IntArray::length() const{
    //b-a = size
    return (b-a);
}
//removed const declarations
ostream & operator << (ostream & output, IntArray & array) {
    for (int i = array.low(); i <= array.high(); i++) {
        output << array.name << "[" << i << "] = " << array[i] << endl;
    }
    return output;
}
//removed const declarations
IntArray & IntArray::operator = (IntArray & right) {
    if (length() == right.length()) {
        for (int i = 0; i <= length(); i++) {
            num[i] = right[right.low()+i];
        }
    return * this;
    }
    else {
        delete [] num;  //reclaim space
        delete [] name;
        size = right.length();
        num = new int [size]; //space created
        cout << "Different sized arrays. Simulating a hault" << endl;
    }
    return * this;
}
//removed const declarations
IntArray & IntArray::operator + (IntArray & right) {
    int * ptr;
    ptr = new int [right.length()];
    if (length() == right.length()) {
        for (int i = 0; i < length(); i++) {
            ptr[i] = num[i] + right[right.low()+i];
        }
    }
    return * this;
}
//removed const declarations
bool IntArray::operator += (IntArray & right) {
    if (length() == right.length()) {
        for (int i = 0; i <= right.length(); i++) {
            num[i] += right[right.low()+i];
        }
        return true;
    }
    cout << "Could not add the sum of the arrays into first array. Simulating a halt." << endl;
    return false;
}

iadrv.h

//  iadrv.h

#ifndef p6_iadrv_h
#define p6_iadrv_h

#include "intarray.h"

int main();
void test1();
void wait();

#endif

iadrv.cpp

//  iadrv.cpp

#include <iostream>
#include <iomanip>
#include <fstream>
#include <stdlib.h>
#include "iadrv.h"

using namespace std;

ofstream csis;

int main() {
    csis.open("csis.dat");
    test1();
    csis.close();
}

void test1() {
    system("clear");
    cout << "1. Array declared with single integer: IntArray a(10);" << endl << endl;
    csis << "1. Array declared with single integer: IntArray a(10);" << endl << endl;
    IntArray a(10);
    for(int i = a.low(); i <= a.high(); i++)
        a[i] = i * 10;
    a.setName("a");
    cout << a << endl;
    csis << a << endl;
    wait();
}

DISCLAIMER: This program was written as a school assignment and has already been turned in to be graded. This was my first c++ program so I would like to understand my mistakes. Your help is sincerely appreciated.

cjan92127
  • 15
  • 1
  • 1
  • 5
  • Your `operator=` is too complex. It could be written much more simpler than what you attempted. In addition, your `operator+` could be written to just call `operator +=`. – PaulMcKenzie Nov 21 '14 at 04:27

3 Answers3

1

You have defined your operator[] like this:

const int operator [] (int) const;

that second "const" means that inside that method you cannot modify your object.

So it will only work for getting values, but not for setting them.

Try to remove it and it should work.

EDIT: AS pointed to Bryan Chen, You also need to return a reference and non-const, like this:

int& operator [] (int subscript)

Now, looking more in depth at your code, that is not even enough, because you have this method:

ostream & operator << (ostream & output, const IntArray & array) {
    for (int i = array.low(); i <= array.high(); i++) {
        output << array.name << "[" << i << "] = " << array[i] << endl;
    }
    return output;
}

Look that you operator[] needs to work on a non-const IntArray, but in that method your variable "array" is const, so you need to rewrite a bit more of code.

Also, look for the same problem with the rest of the operators, remember: you make a method 'const' only if you don't plan to modify the object from inside that method, and you make a parameter 'const' only if you don't plan to modify that parameter.

Daniel
  • 21,933
  • 14
  • 72
  • 101
  • that's not enough. you should provide overload that return `int&`. and first const doesn't make any sense – Bryan Chen Nov 21 '14 at 04:13
  • @DWilches I removed the second "const" from the operator[]. However, my ostream and operator= functions now complain, "No viable overloaded operator[] for type 'const IntArray'. Do I need to remove the const declaration from all of these functions? – cjan92127 Nov 21 '14 at 04:18
  • @BryanChen can you give an example of returning an int&? I thought I was doing this by declaring variable `name` as a pointer and returning its' value. – cjan92127 Nov 21 '14 at 04:21
  • Typically you will have two `operator[]` overloads, one const and one not: `int operator[](int index) const; int & operator[](int index);`. This provides a read-write non-const implementation, and a read-only const implementation. – cdhowie Nov 21 '14 at 04:26
  • @DWilches okay I will try these updated. I originally included "const" because the lab requires that "once an IntArray object has been created, its size cannot be changed". Will removing "const" from operator[] and ostream still allow this to be done? – cjan92127 Nov 21 '14 at 04:26
  • @cjan92127 Not unless you write code that allows it to be expanded this way. The way `std::array` and `std::vector` approach this is that indexing with an out-of-bounds index is undefined behavior. A const method just isn't allowed to change the value of the object. The elements of the array are values, so you need a non-const method to change those. That doesn't mean that *everything* about the object's value is automatically mutable, only what you provide a mechanism to make mutable. – cdhowie Nov 21 '14 at 04:27
  • @cdhowie okay that makes sense. Can I leave my code as is in that case and simply add the additional non-constant operator[] overload? – cjan92127 Nov 21 '14 at 04:28
  • If inside your operator[] you only assign to already existing elements, then the size will not change (I mean, if the index doesn't exist, don't create it, just throw an exception) – Daniel Nov 21 '14 at 04:28
  • @cjan92127 Kind of. You should return `int` instead of `const int` though. Returning a const value doesn't make a lot of sense, and can make it impossible for the compiler to elide copies of more complex types for no benefit. (When you return by value you are returning a copy anyway, so there is no reason to return it const.) – cdhowie Nov 21 '14 at 04:30
  • @Dwilches I removed "const" from most of the functions (see post for updated code). I do not get the assignment error anymore! However, I now see an error "ISO C++ 11 does not allow conversion from string literal to 'char *'" when in the statement `a.setName("a"); in iadrv.cpp...Any ideas? – cjan92127 Nov 21 '14 at 04:48
  • It is a const problem, if you pass "a" then you must receive a `const char*` , not a `char*` – Daniel Nov 21 '14 at 04:50
0

Your existing operator does not allow the value to be changed, both because it returns an int by value and because the operator is declared const. You can't assign to a value, only to an object (which includes references, since a reference is just another name for an object).

To accomplish this you need to supplement your existing operator with another, non-const one that returns a reference to the (non-const) int:

int & operator[](int index);

Since this operator would return a reference, you can assign directly to the return value using the familiar a[b] = c syntax you desire to use.

You would not need to change your existing operator, but I would strongly recommend changing the return type from const int to just int -- you are returning by value anyway, so you are handing back a copy. It makes no sense for this to be const, and this may prevent the compiler from eliding copies in the case of more complex data types than int. (It doesn't really make much difference here, but I would avoid getting in the habit of returning both by value and const, since -- assuming the presence of a copy constructor -- the const qualifier can be removed anyway by simply copying the value again. Returning const copies usually provides no benefits while having several drawbacks.)

cdhowie
  • 158,093
  • 24
  • 286
  • 300
0

Since you also asked to point out your mistakes, I would like to comment on two things you should/could have done to make the code more simple:

First, the assignment operator could have been written like this:

IntArray& operator=(IntArray rhs)
{
   std::swap(rhs.a, a);
   std::swap(rhs.b, b);
   std::swap(rhs.size, size);
   std::swap(rhs.num, num);
   std::swap(rhs.name, name);
   return *this;
}

This works, since you have a copy constructor and destructor already defined for IntArray, and they hopefully work correctly. All the assignment operator is doing is creating a temporary object and swapping out its internals with the internals of the current object. Then the temporary dies with the "old data", while the new data is safely in the current object. This is called the copy/swap idiom.

Also note that a reference is returned that is non-const.

If you pass a const reference instead of an object, then the assignment operator is responsible for the initial copy made.

IntArray& operator=(const IntArray& orig)
{
   IntArray rhs(orig);
   std::swap(rhs.a, a);
   std::swap(rhs.b, b);
   std::swap(rhs.size, size);
   std::swap(rhs.num, num);
   std::swap(rhs.name, name);
   return *this;
}

The former version may be faster, due to allowing the compiler to optimize the copy of the passed value. However the second form of passing a const reference is what is usually done -- note that the temporary object needs to be created inside the function before proceeding.

Second, your operator + can just use operator +=:

IntArray operator+(const IntArray& rhs)
{
   IntArray temp(*this);
   return temp += rhs;
}

All we did was create a temporary equal to the current object. Then we use += to add rhs and return the result. Nice and simple. Note that operator + returns a new IntArray object, not a const IntArray. In addition, operator += should return a reference to the current object, not bool.

To take advantage of this, your operator += should be rewritten thusly:

IntArray& operator+=(const IntArray& rhs)
{
  //..your current code goes here:
  //...
  return *this;
}

Also, your operator += should not be "erroring out" like that. You need to make the class more robust by attempting to add two IntArrays that may not be the same size. If there really is an error throw an exception. Don't return a bool -- remove the return true; and return false; from the function altogether. Always return *this.

PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45
  • The `IntArray& operator=(IntArray rhs)` pattern is generally considered poor design, I believe. It prevents the fast-no-op where you attempt to assign a value to itself, for example, and it creates an extra intermediate object where one is not strictly necessary. It *will* behave correctly but will perform sub-optimally. It also requires the presence of a copy-constructor, which is not always a correct assumption. – cdhowie Nov 21 '14 at 04:44
  • @cdhowie - There are persons that claim that for assignment operators and copy/swap, passing the object will allow the compiler to do the copy, thus taking advantage of any optimizations that may be present. Also, I am assuming that the IntArray class has a correct copy constructor, which is what I mentioned in the answer (the copy/swap won't work without a working copy constructor). – PaulMcKenzie Nov 21 '14 at 04:47
  • From here is where the research has been done: http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom – PaulMcKenzie Nov 21 '14 at 04:49
  • @cdhowie I do have a copy constructor. Given that, would you still advise against the `IntArray& operator=(IntArray rhs)` pattern? – cjan92127 Nov 21 '14 at 04:52
  • @cjan92127 - You can use what cdhowie suggested by passing a const reference. However, the code needs to be changed slightly, but not by much. The bottom line is that you have a working copy constructor and destructor, so whatever you choose, the copy/swap will work. – PaulMcKenzie Nov 21 '14 at 04:54
  • @cjan92127 Personally I'm not a huge fan of it, but I'm not the authority. Use whichever you feel is most appropriate for your project. – cdhowie Nov 21 '14 at 04:54
  • @PaulMcKenzie this is much appreciated. Do I understand correctly that by including `return temp += rhs;` in `operator +` allows it to call/use the `operator +=` ability to add together IntArrays, then stores the result in variable `temp` which it returns? – cjan92127 Nov 21 '14 at 04:56
  • @cjan92127 - If your `operator +=` returns a reference to the current object, then yes, you can take advantage of it to produce `operator +`. The same thing for other operators such as -=, *=, and /=. – PaulMcKenzie Nov 21 '14 at 05:00