CODEWITHSUNDEEP
c++stl
tpascale
tpascale

Reputation: 2576

STL containers & memory leaks

C# coder just wrote this simple C++ method to get text from a file:

static std::vector<std::string> readTextFile(const std::string &filePath) {
    std::string line;
    std::vector<std::string> lines;
    std::ifstream theFile(filePath.c_str());
    while (theFile.good()) {
    getline (theFile, line);
        lines.push_back(line);
    }
    theFile.close();
    return lines;
}

I know this code is not efficient; the text lines are copied once as they are read and a second time when returned-by-value.

Two questions:

(1) can this code leak memory ? (2) more generally can returning containers of objects by value ever leak memory ? (assuming the objects themselves don't leak)

Upvotes: 1

Views: 1874

Answers (5)

Alexandre C.
Alexandre C.

Reputation: 56956

In C++11, you can do:

std::vector<std::string> 
read_text_file(const std::string& path) 
{
    std::string line;
    std::vector<std::string> ans;
    std::ifstream file(path.c_str());

    while (std::getline(file, line))
       ans.push_back(std::move(line));

    return ans;
}

and no extra copies are made.

In C++03, you accept the extra copies, and painfully remove them only if profiling dictates so.

Note: you don't need to close the file manually, the destructor of std::ifstream does it for you.

Note2: You can template on the char type, this may be useful in some circumstances:

template <typename C, typename T>
std::vector<std::basic_string<C, T>>
read_text_file(const char* path)
{
    std::basic_string<C, T> line;
    std::vector<std::basic_string<C, T>> ans;
    std::basic_ifstream<C, T> file(path);

    // Rest as above
}

Upvotes: 1

Juraj Blaho
Juraj Blaho

Reputation: 13451

No, returning container by value should not leak memory. The standard library is designed not to leak memory itself in any case. It can only leak a memory if there is a bug in its implementation. At least there used to be a bug in vector of strings in an old MSVC.

Upvotes: 0

David Rodr&#237;guez - dribeas
David Rodr&#237;guez - dribeas

Reputation: 208353

(1) can this code leak memory ?

No

(2) more generally can returning containers of objects by value ever leak memory ?

No. You might leak memory that is stored in a container by pointer or through objects that leak. But that would not be caused by returning by value.

I know this code is not efficient; the text lines are copied once as they are read and a second time when returned-by-value.

Most probably not. There are two copies of the string, but not the ones that you are thinking about. The return copy will most likely be optimized in C++03, and will either be optimized away or transformed into a move (cheap) in C++11.

The two copes are rather:

getline (theFile, line);
lines.push_back(line);

The first line copies from the file to line, and the second copies from line to the container. If you are using a C++11 compiler you can change the second line to:

lines.push_back(std::move(line));

to move the contents of the string to the container. Alternatively (and also valid in C++03), you can change the two lines with:

lines.push_back(std::string()); // In most implementations this is *cheap*
                                // (i.e. no memory allocation)
getline(theFile, lines.back());

And you should test the result of the read (if the read fails, in the last alternative, make sure to resize to one less element to remove the last empty string.

Upvotes: 1

Sarfaraz Nawaz
Sarfaraz Nawaz

Reputation: 361422

while (theFile.good()) {
 getline (theFile, line);
    lines.push_back(line);
}

Forget about efficiency, this code is not correct. It will not read the file correctly. See the following topic to know why:

So the loop should be written as:

while (getline (theFile, line)) {
    lines.push_back(line);
}

Now this is correct. If you want to make it efficient, profile your application first. Try see the part which takes most CPU cycles.

(1) can this code leak memory ?

No.

(2) more generally can returning containers of objects by value ever leak memory ?

Depends on the type of the object in the containers. In your case, the type of the object in std::vector is std::string which makes sure that no memory will be leaked.

Upvotes: 5

john
john

Reputation: 87959

No and no. Returning by value never leaks memory (assuming the containers and the contained objects are well written). It would be fairly useless if it was any other way.

And I second what Nawaz says, your while loop is wrong. It's frankly incredible how many times we see that, there must be an awful lot of bad advice out there.

Upvotes: 1

Related Questions