-4

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 != &copyfrom) {

        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
bigcodeszzer
  • 916
  • 1
  • 8
  • 27
  • 1
    Any error message? – T D Nguyen Mar 21 '16 at 21:28
  • 2
    I bet that is NOT a minimal example. – MSalters Mar 21 '16 at 21:28
  • At which point does it crash? – SolaGratia Mar 21 '16 at 21:28
  • 1
    Probably some issue with `w3::Text a;`? – πάντα ῥεῖ Mar 21 '16 at 21:35
  • 3
    Once I remove all of the stuff that can't compile I'm unable to reproduce. Probably a bug in the stuff I had to remove. Are you sure `w3::Text` is Rule of Three compliant? If it isn't then `a = b;` is going to cause you problems. What is The Rule of Three? [Glad you asked!](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – user4581301 Mar 21 '16 at 21:43
  • 2
    *this should be complete* -- It isn't. -- *and utterly impossible* -- it is indeed possible. http://ideone.com/2FSrPm I just added a buggy `w3::Text` class, since you didn't post it. The class I came up violates the Rule of 3, as described by the previous comment. – PaulMcKenzie Mar 21 '16 at 21:55
  • The program is printing done, then crashing before returning zero. – bigcodeszzer Mar 21 '16 at 22:02
  • If there is something wrong with the classes, it should crash before printing. – bigcodeszzer Mar 21 '16 at 22:02
  • @bigcodeszzer My program also crashes before `main` returns 0. It is doing just as the title of your post describes. Also, it does not have to crash before printing. C++ doesn't work that way -- you make a mistake, the program can exhibit undefined behavior. Do you want me to write a buggy `Text` class that crashes? I can do that easily. Why not post what `Text` is and save us from having to fill in the gaps for you? – PaulMcKenzie Mar 21 '16 at 22:04
  • @PaulMcKenzie Is it the main that is crashing or the classes? I copied the main right out of a textbook, one would think it is accurate. – bigcodeszzer Mar 21 '16 at 22:06
  • @bigcodeszzer *I copied the main right out of a textbook,* -- It isn't `main` that's the mystery. The mystery is the `w3::Text` class that you didn't show. – PaulMcKenzie Mar 21 '16 at 22:10
  • Bub, some of the text books I've seen.... You may be able to test with something as simple as `int main() { w3::Text a(argv[1]); w3::Text b(argv[1]); a=b; }` Anyway, I recommend adding `w3::Text` to your question. – user4581301 Mar 21 '16 at 22:10
  • @PaulMcKenzie done........... – bigcodeszzer Mar 21 '16 at 22:14
  • So @user4581301 is exactly right. All you need is a 3 or 4 line `main` program to duplicate the potential bugs. Your class does a whole lot of copying, and if there are bugs in it, just like my simple version, you will get issues. If my simple buggy version can cause it to crash, one that has a hundred or so lines in it has more potential for issues. Also, you should have posted `Text.h`. Also, please read the link on the Rule of 3. You're doing the horrible practice of calling the assignment operator from the copy constructor, when it is way easier to do the opposite. – PaulMcKenzie Mar 21 '16 at 22:22

1 Answers1

2

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.

Community
  • 1
  • 1
user4581301
  • 33,082
  • 7
  • 33
  • 54
  • I've grown more and more accustomed to Java, which makes it harder to use c++. Things crash too often without giving any indication why. – bigcodeszzer Mar 21 '16 at 23:35
  • 1
    @bigcodeszzer Stay away from raw pointers and start to use container classes and smart pointers. Then C++ can become easier to use, almost as easy as Java, since you wouldn't have to write all of this code to maintain a class. If you had done that, you more than likely would not have the issues you were getting. Replacing that `std::string*` member with `std::vector` would be a prime example of using the container classes instead of raw pointers. – PaulMcKenzie Mar 21 '16 at 23:52
  • That makes sense. I am in school though, and they want us to know how to use all of the bare bones stuff first. I'm just psyched I'm allowed to use string instead of char *. – bigcodeszzer Mar 21 '16 at 23:57
  • I have to say I do love Java's ability to give you a detailed stack trace. And I miss the detailed stack dumps I got back in the good ol' days. Ahhhh Windows NT4. Microsoft, how did you go so wrong? Anyway, some IDEs will warn you when you've got members uninitialized after a constructor, and [there's this great tool called Valgrind](http://valgrind.org/) for tracing initialization and memory stomping errors. – user4581301 Mar 22 '16 at 00:03
  • Yeah, Java is pretty awesome. I like valgrind also, but I have only ever used the linux version, it'd be cool if there was an IDE that had it built in. I just use Visual studio. I've been playing around with the idea of base class that uses try/throw catch to trace things, and just inheriting everything from that. – bigcodeszzer Mar 22 '16 at 00:13
  • Visual Studio 2015 pro has some profiling and leak hunting stuff built in that I haven't really had the chance to try out yet. The finally caught up to VS 2010, so now that I have VS 2015, it's probably going to be 5 more years before I'm allowed to write code with it. On the starving student front VS Pro will set you back a few thousand dollars. – user4581301 Mar 22 '16 at 00:20
  • The big Try /catch gag doesn't work in C++. Most of the real nasties, including the one you hit here, are below the exception handler and can't be caught. You will find that you catch pretty much what you expected to catch and miss all of the showstoppers. I recommend log files and getting good with the debugger. – user4581301 Mar 22 '16 at 00:23
  • One of my professors was using an IDE called codelite. I'm pretty sure it supports direct use of valgrind. Probably not as good as Pro, but yeah, definitely not paying that much, unless our school has some licencing somewhere. – bigcodeszzer Mar 22 '16 at 13:08
  • @user4581301 I tested this solution earlier today, and then realized I totally forgot to include the header file, which does initialize data to nullptr and entries to 0. I made the changes in the default constructor anyways, but the program is still crashing. – bigcodeszzer Mar 22 '16 at 19:50
  • I edited the question and included the header file – bigcodeszzer Mar 22 '16 at 19:50