Constructor throws wrong exception

[TSLexi]

Hey guys!

Writing a smart array class for my C++ class. I'm running into a problem where the constructor apparently throws the wrong exception. Compiled on G++ with C++11 extensions enabled.

// headers
#include <iostream>
#include <utility>
#include <cctype>

// stuff we need from namespace std
using std::cout;
using std::cin;
using std::endl;
using std::move;
using std::isalpha;

//exception class
class InvalidSize {};
class InvalidInput {};
// Array class
class Array
{
	public:
	//constructors
	Array()=delete; // Array must have a size, and cannot be stored in a structure.
	Array(int size); //Throws InvalidSize and InvalidInput
	//copy constructor
	Array(const Array& rhs);
	//move constructor
	Array(Array&& rhs);
	//conversion from regular array
	Array(int* array, int size);
	// copy assignment operator
	Array& operator=(const Array& rhs);
	//move assignment operator
	Array& operator=(Array&& rhs);
	//destructor
	~Array();
	//index operator
	int& operator[](int index) const; 
	//conversion to regular array
	operator int*() const;
	//get length of array
	int getsize() const;
	//data fields
	private:
	int size;
	int* array;
};

//main
int main()
{
	// size of array
	int size;
	//ask for array size
    cout << "How big do you want your array to be?" << endl;
    cin >> size;
    try
    {
    	Array array(size);  
    // get data
    cout << "Enter the numbers you'd like to store: " << endl;
    for (int i = 0; i < array.getsize(); i++)
    {
    	int val;
    	cin >> val;
    	array[i]=val;
    }
    //display array contents
    cout << "Displaying array contents." << endl;
    for (int i=0; i < array.getsize(); i++)
    cout << array[i] << endl;
    }
    //handle exceptions
      catch (InvalidSize)
    {
    	cout << "Arrays must have at least two objects!" << endl;
    }
    catch (InvalidInput)
    {
    	cout << "Integers only!" << endl;
    }
 //done
 return 0;
}

//regular constructor
    
  Array::Array(int s): size(s)
  {
  	// validate input
  	if (isalpha(s))
  	throw InvalidInput();
  	else if (s < 2)
  	throw InvalidSize();
  	//otherwise build the array
  	else
  	{
  		cout << "Building array." << endl;
  		array = new int[size];
  	}
  }
  
//copy constructor
Array::Array(const Array& rhs):size(rhs.size), array(new int[size])
{
	//delegate to copy assignment operator
	*this = rhs;
} 

//move constructor
Array::Array(Array&& rhs):array(0)
{
	//delegate to move assignment operator
	*this = move(rhs);
}

//array conversion constructor
Array::Array(int* carray, int s):Array(size)
 {
 	for (int i = 0; i < s; i++)
 	array[i] = carray[i];
 }
 
 //copy assignment operator
 Array& Array::operator=(const Array& rhs)   
 {
 	if (this==&rhs)
 	cout << "Self-assignment prohibited!" << endl;
 	if (size != rhs.size)
 	cout << "Arrays must be equal in size!" << endl;
 	else
 	for (int i = 0; i < size; i++)
 		array[i] =rhs.array[i];
 		return *this;
 }
 
 //move assignment operator
 Array& Array::operator=(Array&& rhs)
 {
 	//not checking for self-assignment, as rhs is likely a temporary
 	if (size != rhs.size)
 	cout << "Arrays must be same size!" << endl;
 	else
 	{
 		cout << "Moving array." << endl;
 		for (int i = 0; i < size; i++)
 			array[i] = rhs.array[i];
 			delete[] rhs.array;
 			rhs.array = 0;
 			return *this;
 	}
 }
 
 //destructor
Array::~Array()
{
	//deallocate the wrapped array
	delete[] array;
	array = 0;
}

//index operator
int& Array::operator[](int index) const
{
	//perform bounds checking
	if (index < 0 || index >= size)
	cout << "Array index must not be negative or greater then or equal to array size." << endl;
	else
	return array[index];
}

//conversion to regular array
Array::operator int*() const
{
	return array;
}

//getter for size
int Array::getsize() const
{
	return size;
}

When I try to check the error-handling code, when I enter a size less then two, Array's ctor throws InvalidSize, and gets caught, all good. But when I enter a letter, the ctor also seems to throw InvalidSize!

Weird. Any advice? Or general comments?

Thanks!

[2kaud]

int size;
...
cin >> size;

As size is defined to be of type int, if you try to enter a non integer value, the cin stream fails and size is not set. You're not checking the input stream for a problem after extracting data. The same applies to when you extract from the input stream the numbers to store. You are not checking the state of the stream to see if an error has occured.

See http://www.cplusplus.com/reference/istream/istream/

[TSLexi]

Ah. I fixed it by

if (cin.fail())
throw InvalidInput();
else
//useful work

every time I used cin. Now, how do I avoid my code becoming a massive nested if-then-else structure of tests and throws?

[Paul McKenzie]

I'm running into a problem where the constructor apparently throws the wrong exception. Compiled on G++ with C++11 extensions enabled.

1) Did you run the program under the debugger to see why it chooses this exception?

Other issues:
1) Your Array::Array(int) constructor is flawed -- what if I tried to create an array of 65 integers? Since 65 is 'a', the isalpha() would have returned 1, causing an exception to be thrown.

2) Your assignment operator has serious side effects -- if you assign an array of a different size, the assignment doesn't fulfill the request. The problem with side-effects is that the compiler can call the assignment operator "behind your back", thus giving you bogus copies floating around that you didn't suspect. The assignment operator (and copy constructor) fulfill one purpose, and that is to assign and copy. Not doing so can raise serious run-time bugs that are difficult to diagnose. When a programmer does this:

T x;
T y;
//...
y = x;

The y should be equivalent and behave exactly the same as x. In other words, after that assignment, if I used y instead of x throughout the program, the program needs to behave exactly the same as if I used x instead of y.

Also, your copy constructor works contextually differently than the assignment -- this leads to confusion when using the class. For example, if I can do this:

T x;
//... assume you added items to x
T y = x;

Then I should be able to do this:

T x;
T y;
// assume you added items to x
y = x;

The first calls the copy constructor, the second calls the assignment operator. Why should the program behavior be different if I use one over the other?

Regards,

Paul McKenzie

[TSLexi]

I rewrote the assignment operators to throw an Out Of Bounds exception when the lhs and rhs do not have the same sizes.

[Paul McKenzie]

I rewrote the assignment operators to throw an Out Of Bounds exception when the lhs and rhs do not have the same sizes.

Which is the wrong thing to do. You don't introduce side-effects in an operation such as assignment, which again, you have no complete control over (unless you turn off assignment altogether).

I also edited my previous post to give another example of why you shouldn't do this.

Regards,

Paul McKenzie

[TSLexi]

So, how should I handle copying between arrays of different sizes?

[Paul McKenzie]

So, how should I handle copying between arrays of different sizes?

In the way you implemented your class, you have to

1) Create new memory with the new size
2) Copy the passed in data to the new memory
3) Delete the old memory

Otherwise, you should have written your copy constructor to do this work, and not delegate it to the assignment operator. Instead, you delegate the assignment operator to call the copy constructor. Then you can do the copy and swap idiom to do the assignment:

Arrray& operator=(const Array& rhs)
{
    Array tmp(rhs);
    swap(*this, tmp);
    return *this;
}

Now you don't even need to test for self-assignment.

http://stackoverflow.com/questions/3...and-swap-idiom

Regards,

Paul McKenzie

[TSLexi]

Rewrote the assignment operators to work properly

//copy assignment operator
 Array& Array::operator=(const Array& rhs)   
 {
 	if (this==&rhs)
 	throw;
 	if (size != rhs.size)
 	{
 		size=rhs.size;
 		delete[] array;
 		array = new int [size];
 	}
 	for (int i = 0; i < size; i++)
 		array[i] =rhs.array[i];
 		return *this;
 }
 
 Array& Array::operator=(Array&& rhs)
 {
 	//not checking for self-assignment as rhs is most likely temporary
 	cout << "Moving array." << endl;
 	
 	// reallocate array size if need be
 	if (size != rhs.size)
 	{
 		size=rhs.size;
 	    delete[] array;
 	    array = new int[size];
 	}
 	for (int i = 0; i < size; i++)
 	array[i] = rhs.array[i];
 	delete[] rhs.array;
 	rhs.array=0;
 	return *this;
 }

[OReubens]

Ah. I fixed it by

if (cin.fail())
throw InvalidInput();
else
//useful work

every time I used cin. Now, how do I avoid my code becoming a massive nested if-then-else structure of tests and throws?

by not writing unnecessary else clauses... for starters...

in the above, the else is not needed. If you enter the if, you get thrown out of the function, so the "else" doesn't really serve any purpose. Doing this rigourously will make your code more readable because the important code that does the real work isn't as deeply indented.

[OReubens]

Another step (with some setup) would be to write thrower-functions.

void ThrowInvalidInput(bool bCondition)
{
   if (bCondition)
        throw InvalidInput();
}


void foo()
{
     // early outs...
     ThrowInvalidInput( cin.fail() );

     // Usefull work
     DrinkBeer();
     EatBagOfCrisps();
     WatchTV();
     ScratchBalls();
}

[TSLexi]

Okay, here's the final version

 // headers
#include <iostream>
#include <utility>
#include <cctype>

// stuff we need from namespace std
using std::cout;
using std::cin;
using std::endl;
using std::move;
using std::isalpha;

//exception class
class InvalidSize {};
class InvalidInput {};
// Array class
class Array
{
	public:
	//constructors
	Array()=delete; // Array must have a size, and cannot be stored in a structure.
	Array(int size); //Throws InvalidSize 
	//copy constructor
	Array(const Array& rhs);
	//move constructor
	Array(Array&& rhs);
	//conversion from regular array
	Array(int* array, int size);
	// copy assignment operator
	Array& operator=(const Array& rhs);
	//move assignment operator
	Array& operator=(Array&& rhs);
	//destructor
	~Array();
	//index operator
	int& operator[](int index) const; 
	//conversion to regular array
	operator int*() const;
	//get length of array
	int getsize() const;
	//data fields
	private:
	int size;
	int* array;
};

//Thrower function for input validation
void IsInputValid(bool condition);

//main
int main()
{
	// size of array
	int size;
	//ask for array size
    cout << "How big do you want your array to be?" << endl;
    cin >> size;
    IsInputValid(cin.fail());
    try
    {
    	Array array(size);  
    // get data
    cout << "Enter the numbers you'd like to store: " << endl;
    for (int i = 0; i < array.getsize(); i++)
    {
    	int val;
    	cin >> val;
    	IsInputValid(cin.fail());
    	array[i]=val;
    }
    //display array contents
    cout << "Displaying array contents." << endl;
    for (int i=0; i < array.getsize(); i++)
    cout << array[i] << endl;
    }
    //handle exceptions
      catch (InvalidSize)
    {
    	cout << "Arrays must have at least two objects!" << endl;
    }
    catch (InvalidInput)
    {
    	cout << "Integers only!" << endl;
    }
 //done
 return 0;
}

//regular constructor
    
  Array::Array(int s): size(s)
  {
  	// make sure array has at least two objects
  	 if (s < 2)
  	throw InvalidSize();
  	//otherwise build the array
  	else
  	{
  		cout << "Building array." << endl;
  		array = new int[size];
  	}
  }
  
//copy constructor
Array::Array(const Array& rhs):size(rhs.size), array(new int[size])
{
	//delegate to copy assignment operator
	*this = rhs;
} 

//move constructor
Array::Array(Array&& rhs):array(0)
{
	//delegate to move assignment operator
	*this = move(rhs);
}

//array conversion constructor
Array::Array(int* carray, int s):Array(size)
 {
 	for (int i = 0; i < s; i++)
 	array[i] = carray[i];
 }
 
 //copy assignment operator
 Array& Array::operator=(const Array& rhs)   
 {
 	if (this==&rhs)
 	throw;
 	if (size != rhs.size)
 	{
 		size=rhs.size;
 		delete[] array;
 		array = new int [size];
 	}
 	for (int i = 0; i < size; i++)
 		array[i] =rhs.array[i];
 		return *this;
 }
 
 Array& Array::operator=(Array&& rhs)
 {
 	//not checking for self-assignment as rhs is most likely temporary
 	cout << "Moving array." << endl;
 	
 	// reallocate array size if need be
 	if (size != rhs.size)
 	{
 		size=rhs.size;
 	    delete[] array;
 	    array = new int[size];
 	}
 	for (int i = 0; i < size; i++)
 	array[i] = rhs.array[i];
 	delete[] rhs.array;
 	rhs.array=0;
 	return *this;
 }
 
 //destructor
Array::~Array()
{
	//deallocate the wrapped array
	delete[] array;
	array = 0;
}

//index operator
int& Array::operator[](int index) const
{
	//perform bounds checking
	if (index < 0 || index >= size)
	cout << "Array index must not be negative or greater then or equal to array size." << endl;
	else
	return array[index];
}

//conversion to regular array
Array::operator int*() const
{
	return array;
}

//getter for size
int Array::getsize() const
{
	return size;
}

void IsInputValid(bool condition)
{
	if(condition)
	throw InvalidInput();
	else
	return;
}

When I enter a letter, the InvalidInput exception doesn't seem to get caught by the corresponding catch, but instead the OS catches it, thus calling terminate(). Any ideas as to why?

Never mind...try block didn't cover the first call of IsInputValid().

[Paul McKenzie]

Okay, here's the final version

Why does your assignment operator throw if it's self assignment? Just make it a no-op.

Array& Array::operator=(const Array& rhs)   
{
    if (this == &rhs)
       return *this;
   //...
}

Also, you need two overloads of operator [] to cover both left and right hand side assignment if [] appears on either side. For example, your array class fails if I did this:

Array a(2);
//...
a[0] = a[1];

The left hand side is non-const, since it changes the internals of the array, but you do not have a non-const overload.

You also have a bug here:

//array conversion constructor
Array::Array(int* carray, int s):Array(size)

Last, what happens if new throws here?

if (size != rhs.size)
{
   size=rhs.size;
    delete[] array;
    array = new int[size];
 }

You deleted the array before allocating with new[]. If new[] now throws an exception, your array is in an invalid state, as well as messing up the size variable (it has the size of the rhs array instead of retaining the original value). That's why you should first allocate the memory (create a temp pointer), copy the data to this memory, then do the deletion of the array, and last, assign the temp pointer to the array. If the allocation is successful, then you change the other members, such as size.

if (size != rhs.size)
{
   int *temp = new int [rhs.size];
   size = rhs.size;  // ok to change now.

   // here you now copy the data to temp
   //...
   // now you delete array and assign temp to array
   delete [] array;
   array = temp;
}

Also, this is the reason by making the copy constructor do this work, and then delegating the assignment operator to call the copy constructor (and not the other way around, as your code is doing), makes life easier -- you don't need to check for self-assignment, and the code becomes automatically exception safe due to the copy-swap idiom I mentioned earlier.

Regards,

Paul McKenzie

[TSLexi]

Why does your assignment operator throw if it's self assignment? Just make it a no-op.

 Array& Array::operator=(const Array& rhs)   
 {
 	if (this == &rhs)
           return *this;
   //...
 }

Regards,

Paul McKenzie

Just remembered that! Haven't written copy assignment operators for a while. BTW, is this basically how std::array is written?

[laserlight]

is this basically how std::array is written?

No, std::array is a class template in which the array size is specified at compile time and its copy assignment operator is implicitly declared.