I'm writing a program in C++ and have encountered the most bizarre error. My program crashes right before main is to return 0.
I do not understand how the program can crash after finishing and yet before returning zero.
How is this possible?
The main is the following:
#include <iostream>
#include <iomanip>
#include <utility>
#include <ctime>
#include "Text.h"
#define TIME(start, end) double((end) - (start)) / CLOCKS_PER_SEC
int main(int argc, char* argv[]) {
argv[1] = "gutenberg_shakespeare.txt";
argc = 2;
if (argc == 1) {
std::cerr << argv[0] << ": missing file operand\n";
return 1;
}
else if (argc != 2) {
std::cerr << argv[0] << ": too many arguments\n";
return 2;
}
std::clock_t cs, ce;
std::cout << std::fixed << std::setprecision(3);
cs = std::clock();
w3::Text a;
ce = std::clock();
std::cout << "Constructor " << TIME(cs, ce) << " seconds";
std::cout << " - a.size = " << a.size() << std::endl;
cs = std::clock();
w3::Text b(argv[1]);
ce = std::clock();
std::cout << "Constructor " << TIME(cs, ce) << " seconds";
std::cout << " - b.size = " << b.size() << std::endl;
cs = std::clock();
a = b;
ce = std::clock();
std::cout << "Copy Assignment " << TIME(cs, ce) << " seconds";
std::cout << " - a.size = " << a.size() << std::endl;
cs = std::clock();
a = std::move(b);
ce = std::clock();
std::cout << "Move Assignment " << TIME(cs, ce) << " seconds";
std::cout << " - a.size = " << a.size() << std::endl;
cs = std::clock();
w3::Text c = a;
ce = std::clock();
std::cout << "Copy Constructor " << TIME(cs, ce) << " seconds";
std::cout << " - c.size = " << c.size() << std::endl;
cs = std::clock();
w3::Text d = std::move(a);
ce = std::clock();
std::cout << "Move Constructor " << TIME(cs, ce) << " seconds";
std::cout << " - d.size = " << d.size() << std::endl;
cs = std::clock();
ce = std::clock();
std::cout << "Destructor " << TIME(cs, ce) << " seconds\n";
std::cout << "DONE";
return 0;
}
The text cpp file:
#include "Text.h"
#include <fstream>
#include <iostream>
using namespace w3;
Text::Text() {
}
//MOVE Copy constructor
Text::Text(Text&& movefrom){
std::cout << "MOE COPY CONSTRUCTOR" << std::endl;
filename = movefrom.filename;
entries = movefrom.entries;
data = movefrom.data;
movefrom.entries = 0;
movefrom.data = nullptr;
}
//move assig operator
Text&& Text::operator=(Text&& movefrom) {
std::cout << "move assig operator" << std::endl;
if (&movefrom != this) {
filename = movefrom.filename;
entries = movefrom.entries;
if (data != nullptr) {
delete[] data;
entries = 0;
}
movefrom.data = nullptr;
}
movefrom.entries = 0;
return std::move(*this);
}
//constructor
Text::Text(const std::string& mystring) {
std::cout << "Constructor" << std::endl;
int count = 0;
filename = mystring;
std::string buffer;
std::ifstream myfile(filename);
if (!myfile.is_open()) {
filename.clear();
}
if(myfile.is_open()) {
while (getline(myfile, buffer)) { //Will fail at end of file
//std::cout << buffer << std::endl;
count++;
}
std::cout << "File is read";
data = new std::string[count];
myfile.clear();//.................reset file state
myfile.seekg(0, myfile.beg);//....reset file position
int x = 0;
for (int i = 0; i < count; i++) {
getline(myfile, data[i]);
}
std::cout << std::endl << "File is copied" << std::endl;
entries = count;
myfile.close();
}
}
//default constructor
Text::~Text() {
if (data != nullptr) {
delete[] data;
entries = 0;
}
data = nullptr;
}
//copy constructor
Text::Text(const Text& copyfrom) {
data = nullptr; //The object is empty
*this = copyfrom;
}
const Text& Text::operator=(const Text& copyfrom) {
std::cout << "copy assign operator" << std::endl;
if (this != ©from) {
if (data != nullptr) {
delete[] data;
entries = 0;
}
filename = copyfrom.filename;
entries = copyfrom.entries;
if (copyfrom.data != nullptr) { //If the object is not empty
data = new std::string[entries];
for (int i = 0; i < entries; i++) {
data[i] = copyfrom.data[i];
}
}
std::cout << "Data is assigned" << std::endl;
}
return *this;
}
size_t Text::size() const {
return entries;
}
void Text::print() {
for (int i = 0; i < entries; i++) {
std::cout << data[i] << std::endl;
}
}
EDIT >>> The header file
#ifndef TEXT_H
#define TEXT_H
#define FILE_LENGTH 10
#include <string>
#include <iostream>
namespace w3 {
class Text {
private:
std::string filename;
std::string * data = nullptr;
size_t entries;
public:
Text(Text&& movefrom);
Text&& operator=(Text&& movefrom);
Text();
Text(const std::string& mystring);
Text(const Text& copyfrom);
~Text();
const Text& operator=(const Text& copyfrom);
size_t size() const;
void print();
};
}
#endif
I have to say, the failure is not what I was expecting. Kudos to OP on that. I expected a double delete because two objects pointed to the same pool. So this isn't a run-of-the-mill Rule of Three violation; it's a bit different.
Quick glance says the copy logic is good (consider using std::vector for data if you can' You can throw out about 1/4 of your code if you do.) and the big death point is an incomplete default constructor.
w3::Text a;
Will call the bare-bones default constructor
Text::Text() {
}
Which does not initialize data to nullptr. This causes problems here in const Text& Text::operator=(const Text& copyfrom):
if (data != nullptr)
{
delete[] data;
entries = 0;
}
when a=b; a.data was never set, is unlikely to be nullptr and will attempt to free storage it doesn't own. If the program manages to limp through this, and it could, it's state is now invalid and Crom only knows when it will fail.
Text::Text(): entries(0), data(nullptr)
{
}
solves that.
Also...
The copy constructor leaks
Text::Text(const Text& copyfrom)
{
data = nullptr; //The object is empty
*this = copyfrom;
}
How do you know data is empty? There is no test, so Poof! If there was anything there, it's unreachable now. But as Paul McKenzie notes above, it's almost always better to write the assignment operator in terms of the Copy constructor.
The move assignment operator is a bit wonky.
Text&& Text::operator=(Text&& movefrom)
should probably be
Text& Text::operator=(Text&& movefrom)
That means you don't have to clear this with return std::move(*this);. Just return *this;
In the destructor there is no point to
data = nullptr;
The object is destroyed immediately after that line, so nulling data is wasted effort.