Thread: Using fstream pointers

Recall that this already attempts to open the file:

Code:

ifstream File(path);

Therefore, you should remove this line:

Code:

f_ptr->open(path);

Then, I would rework your file reading: in general, we do not use eof() to control a loop when we have better options, e.g.,

Code:

string line;
while (getline(*f_ptr, line)) {
    stringstream stream(line);
    copy(istream_iterator<string>(stream), istream_iterator<string>(), back_inserter(words));
}

This is wrong because you did not use new:

Code:

delete f_ptr;

You should remove it.

EDIT:
I wonder if you really need the vector of file stream pointers. After all, since you are reading the files sequentially, you can open a file, read, close it, open the next file, read, close it, etc. At most you would need a vector of strings to store the file paths.

> I'm using a vector of ifstream objects
Bear in mind that most systems have quite a low limit on the number of concurrently open files.

You can have as many filenames as you like in a vector<string>, up to your memory limits. If you intend to process them sequentially anyway, there is no point in opening them all in advance.

Also bear in mind that concurrent filesystem access is usually slow. Before you go crazy on the parallelization, check that it's actually giving you a decent speedup instead of just making your program more complex.

Originally Posted by sean_cantab

(a) the problem was indeed not dereferencing the fstream pointer but in multiple file open attempts. However I'm puzzled that when we don't use any pointers but fstream objects throughout such multiple attempts to open() (as seen in my Pastie code, OP) don't seem to have an adverse / any effect on the program output

I do not see any open() in your original pasted code.

...I thought it would also be a good exercise in learning to use smart pointers

So continuing with the pointer based approach (if you'd kindly indulge me a bit longer)...

That's cool.

So your code is lacking in the indentation department. The opening and closing brace should on the same indentation level. Better to also use Allman style to make it extra clear where your scope starts and ends. Therefore I've taken the liberty of cleaning up the code because I couldn't read it due to the misleading indentation:

Code:

#include <iostream>
#include <fstream>
#include<vector>
#include<memory>
#include<string>
#include<sstream>

auto constexpr num_files = 2;
using namespace std;

int main()
{

	vector<ifstream*> Files;
	vector<string> words;
	int i{};

	while (i < num_files)
	{
		stringstream filename;
		filename << "F:\\test" << i << ".txt";
		ifstream* f = new ifstream(filename.str());
		Files.push_back(move(f)); //push_back (w/o move) => 2 copies of f, the original and the other as element of the vector;
		f = nullptr; // since f has been moved it's internals are undefined and hence reset;

		if (Files[i]->is_open())
		{
			while (!Files[i]->eof()) 
			{
				string line;
				while (getline(*(Files[i]), line))
				{
					stringstream stream(line);
					while (stream >> line)
					{
						words.emplace_back(line);
						//emplace_back instead copy, back_inserter reduces need to #include algo and iterator
					}
				}
			}
			Files[i]->close();//=>only one file is open at a time;
		}

		delete f;
		delete Files[i];//since element i was moved, not copied, into vector Files, is this still necessary?
		i++;
	}

	for (auto& itr : words)
	{
		cout << itr << "\n";
	}
}

The main question I have with this program is if the raw pointers are placed in the vector<ifstream*> with push_back(std::move()) (line 18 of the code), do I still need to delete the individual elements of the vector<ifstream*>. I've done this in line 34 of the code but not entirely sure and my gut feel is that I don't need to. I did delete the temp pointer, f, that was used to assign the fstream objects in the first instance (line 33)

1) Moving a pointer is the same as copying it. Moving it does not set it to null, so moving pointers will only serve to confuse you.
I'm going to give you an analogy. Then you answer my questions how you think it would make sense.

Let's say there's a world broker. By asking this broker, you can rent a house (for free). So when you rent a house, you can do whatever you want with it. However, as long as it's rented, no one else you can use it. So you must tell the broker when you're finished renting the house so the broker can lend it to someone else in the future. Make sense? Good. Now, when you rent a house, the broker picks one at random and gives you the address to that house. When you're finished renting that house, you must give the address back to the broker and you must do it only ONCE. If you do it more than once, it's undefined behavior.

So at line 22 you rent a new house.
At line 23, you move address into the vector.
So tell me: For each iteration, how many houses do you rent? How many times do you need to tell the broker that you want to stop renting said house?
You stop renting a house by calling delete.

Now, moving on to smart pointers, the main problem I'm facing here is assigning the ifstream object to the unique_ptr (I'm assuming this is better suited than shared_ptr for present case?). Before presenting the entire code I'll show my two separate attempts and corresponding error messages:

Do you understand the difference between the concepts of unique_ptr and shared_ptr?

Code:

unique_ptr<ifstream> upf = new fstream(filename.str());
error: conversion from 'std::fstream* {aka std::basic_fstream<char>*}' to non-scalar type 'std::unique_ptr<std::basic_ifstream<char> >' requested

You can't assign a raw pointer to a unique_ptr. It also has an explicit constructor, so you must use the initialization syntax:

Code:

unique_ptr<ifstream> upf{ new fstream(filename.str()) }

If you later want to initialize the smart pointers, you must construct a temporary object:

Code:

upf = unique_ptr<ifstream>{ new fstream(filename.str()) }

Btw, the error says it can't convert fstream* to unique_ptr<fstream>, because that's the closes match among its assignment operators.

myFiles_ptr[i] ( new fstream(filename.str()));
error: no match for call to '(__gnu_cxx::__alloc_traits<std::allocator<std::un ique_ptr<std::basic_ifstream<char> > > >::value_type {aka std::unique_ptr<std::basic_ifstream<char> >}) (std::fstream*)'[/code]

This is a function call, which is wrong no matter how you look at it.
The error message tries to tell you this:
(__gnu_cxx::__alloc_traits<std::allocator<std::uni que_ptr<std::basic_ifstream<char> > > >::value_type is the function name.
std::fstream* is its arguments.

Also, since you're using C++11, you should use nullptr, not NULL.

Originally Posted by sean_cantab

(b) the eof() however seems required, otherwise the program just reads the first line

That doesn't make sense, until you erroneously changed your code from:

Code:

if (f_ptr->is_open()) {
    while (!f_ptr->eof()) {
        string line;
        getline(*f_ptr, line);
        stringstream stream(line);
        copy(istream_iterator<string>(stream), istream_iterator<string>(), back_inserter(words));
    }
}

to:

Code:

if (f_ptr->is_open()) {
    string line;
    getline(*f_ptr, line);
    stringstream stream(line);
    copy(istream_iterator<string>(stream), istream_iterator<string>(), back_inserter(words));
}

in which case of course the program just reads the first line, not because you're not calling eof(), but because you're not calling getline in a loop. What I proposed would be:

Code:

if (f_ptr->is_open()) {
    string line;
    while (getline(*f_ptr, line)) {
        stringstream stream(line);
        copy(istream_iterator<string>(stream), istream_iterator<string>(), back_inserter(words));
    }
}

in which case the program will read line by line, contrary to your claim that eof() is required otherwise the program just reads the first line. Actually, since you are going to parse for "words" anyway, you don't need getline at all. You might as well write:

Code:

if (f_ptr->is_open()) {
    copy(istream_iterator<string>(*f_ptr), istream_iterator<string>(), back_inserter(words));
}

Originally Posted by Elysia

I do not see any open() in your original pasted code.

Line 16.

Originally Posted by sean_cantab

1. if I understand your analogy correctly vis-a-vis my code, house == filestream object == fstream(filename.str()) and address == filestream pointer == f. At each iteration, one house (one filestream object) is rented (i.e address received for) and therefore one house has to be stopped being rented (give the address back to the broker) and so line 45 in your cleaned-up, indented code of #6 needs to be removed

Think about this a little more closely.

On line 22, where does f point?
On line 23, where does f point? Where does Files.back() point?
On line 24, where does f point? Where does Files.back() point?
On line 44, where does f point? Where does Files.back() point?
On line 45, where does f point? Where does Files.back() point?

2. I am learning the concepts of unique_ptr, shared_ptr and in this example, based on what I've read so far, I'd ideally have liked to use unique_ptr. However make_unique is a C++14 feature that my compiler doesn't support and in the code below I've used make_shared instead. I also found it difficult to use unique_ptr with push_back as, I understand, it involves copying the pointers which is not supported by the unique_ptr ctors. However, I remain cautious about making too many statments about smart pointers as its my early days yet with these beasts

You are right that you can't copy a unique_ptr. It doesn't really matter that you can't use make_unique. You can just create it via the constructor as with any other object:

Code:

Files.push_back(std::unique_ptr<MyType>{ ... });

Or...

Code:

std::unique_ptr<MyType> p{ ... };
Files.push_back(std::move(p));

But what I'd like to know is why you chose a unique pointer. What is your reasoning?
What is the difference between a unique pointer and a shared one?

@Elysia: I've re-looked at the code to try and answer your questions:

On line 22, where does f point?
f: ifstream object filename.str();
On line 23, where does f point? Where does Files.back() point?
f: undefined upon move; Files.back(): ifstream object filename.str();
On line 24, where does f point? Where does Files.back() point?
f: nullptr => address 0; Files.back(): ifstream object filename.str();
On line 44, where does f point? Where does Files.back() point?
f: just prior to delete f is still the nullptr; Files.back(): ifstream object filename.str();
On line 45, where does f point? Where does Files.back() point?
f: has been deleted in line 44; Files.back(): just prior to delete, to ifstream object filename.str(), so if we delete Files.back() we lose all future access to this object

unique_ptr gives unique access to the underlying object while there can be multiple shared_ptr's pointing to the same object. In this example I wanted one pointer to open one and only one file and hence I was leaning towards unique_ptr. Here's the code using unique_ptr: #10953028 - Pastie

And finally, after having gone mano-a-mano with this issue over the past two days I've come to the startling (at least for me) conclusion that I didn't really need any vectors of ifstream objects or pointers to read multiple files sequentially. A single ifstream object can just as well loop through the files! Code is here for those interested: #10953032 - Pastie

Many thanks for your kind attention and help.

You are right, except on a few things. You seem to be under some impression that you need to delete a pointer. The pointer just contains the address that the broker gave us. You can't "delete" a pointer. Another thing you seem confused about is that moving a pointer is the same as copying a pointer, so f is not undefined; it contains the same value as Files.back(). It only changes value when you set it to null.

So we are in agreement that we need to delete once. So which line does it make sense to remove then?

Right, so you seem to somewhat understand the semantics of the smart pointers. More generally, unique_ptr models single (or unique) ownership. When the owner is done with the object, it's freed. shared_ptr models multiple ownership, and the idea here is that the last owner frees the object when it no longer needs it. Hence, it does make sense to use unique_ptr here because we're not indenting to share the ownership of the file streams with anyone else. Hence, you shouldn't use make_shared.

Also, unless you intend to reuse the same filestream object, you don't need to close it. That happens automatically when the object goes out of scope.

Also, what compiler are you using (remembering compiler != IDE)? (Sorry if you already mentioned it in another topic)
If it's GCC/MinGW (some older version is bundled with Code::Blocks, even older with DevC++ - the latter you probably should avoid as the plague...) - then try adding -std=c++14 to your compile line. Might just work. (Also, -pedantic -Wall -Wextra are good to force the compiler to show more useful warnings, and -Wfatal-errors to stop on the first error - useful in case of long template error messages where the first one is usually the most useful one, and the rest is just barely useless gibberish cause by the ripple effect.
Also, there's MinGW/GCC 6.1.0 that supports some C++17 stuff, link in my signature (Nuwen's MinGW distro). It's maintained by a trustworthy guy, even though he works at Microsoft. :P

More on topic - learning pointers and smart pointers is a commendable goal, but this whole example was just unnecessarily convoluted by introducing them. As you in the end realized - just using a plain object here was sufficient and a whole lot clearer solution.


However, in the spirit of learning, have a few very basic problems that you'll no doubt solve right of the bat:
1. Consider the following snippet:

Code:

bool is_special(std::string* s) {
  bool result = *s == "special";
  delete s;
  return result;
}

What's wrong with this code? How would you improve on it?

2. Consider the following snippet:

Code:

struct Coord { float x, y; };
//...
std::cin >> size;
Coord* coords = new Coord[size];

for (int i = 0; i < size; ++i) {
    coords[i].x = ...;
    coords[i].y = ...;
}
//...
delete[] coords;

How would you improve on it?

3. Consider the following listing:

Code:

#include <iostream>
#include <string>
#include <vector>

class Animal {
  public:
    virtual std::string vocalize() const = 0;
    virtual ~Animal() { }
};

class Dog : public Animal {
  public:
    virtual std::string vocalize() const {
        return "Woof!";
    }
    virtual ~Dog() { }
};

class Cat : public Animal {
  public:
    virtual std::string vocalize() const {
        return "Meow!";
    }
    virtual ~Cat() { }
};

int main() {
    std::vector<Animal*> pack;
    
    for (int i = 0; i < 10; ++i) {
        if (i % 2) {
            pack.push_back(new Dog);
        } else {
            pack.push_back(new Cat);
        }
    }
    
    for (auto& animal: pack) {
        std::cout << animal->vocalize();
    }
    
    for (auto& animal: pack) {
        delete animal;
    }
}

Again, how do you think this could be improved upon?

4. Consider the following incomplete example code:

Code:

class HeavyFrame {
    std::unique_ptr<int[]> data;
    //...
  public:
    const int* get_data() const { return data.get(); }
};
//...
HeavyFrame frame;
frame.load(device_letting_you_access_const_data);
frame.work_on_data();
legacy_library_function(frame.get_data(), frame.size());

Again, how would you improve on it?

Quick note on #2: If you allocate an array in a unique_ptr, you must use the T[] syntax, e.g.

Code:

std::unique_ptr<int[]> p{new int[10]};

A note on #4: unique_ptr means you can't make a copy, and by returning a new unique_ptr, you do just that, so it won't compile. You should determine who has ownership and who does not. Typically one returns a raw pointer if data must be shared, but the receiver does not share ownership.

do you mean: Settings->Compiler->Other Compiler Options ... where a large text box appears

I don't have Code::Blocks at hand at the moment, but... Probably yes.

should I just copy/paste -std=c++14 here?

Assuming it's the right place - yes. There should be some place to show you what options you have applied at the moment. (Then again, it's been some time - maybe it shows you in the logs only.)

With C::B you can update your GCC using the link I gave you, but if it works - it works. I'm running clang and GCC 6.1.0 at the moment, so I could somewhat assist if need be.


As for the exercises:
#1 Good. Now the function is working fine and doesn't delete memory that it shouldn't touch. However, you shouldn't be shy - you can still make that code better. You can modify any part of it you deem necessary here. So, you improved it - that's good. But it's still not as good as it could be. Can you make it even better?

#2 Elysia is right in that if you want to allocate an array with new[] then and hold a pointer to it in a std::unique_ptr - you need to:
- preferably: use the array notation when supplying it the type:

Code:

std::unique_ptr<int[]> coords(new int[size]);

- OR, if you for whatever reason can't do that, or like to make your life harder - supply a custom deleter in the form of a functor, lambda, or a standard solution:

Code:

std::unique_ptr<int> coords(new int[size], std::default_delete<int[]>());

Otherwise the smart pointer will show you that it's not THAT smart and will try to apply delete instead of delete[] - and that is wrong for a number of reasons.
However, can you think of even cleaner, more basic, solution? Without smart pointers maybe? ; )

#3 You got that one. You still need to work a tad on your indentation, but other than that - this one you can consider done. : )

#4 (under the listing I found: "404 characters / 13 lines", haha! Well, gee, is the universe trying to tell you something? ; ) )
This one was intentionally tricky, and depended on making reasonable assumptions about the (missing) code, I admit. I wanted to throw you a curved ball to see if you won't somewhat blindly try to replace every occurrence of a raw pointer to a smart pointer. However, can you try and explain why you deemed raw pointer there to be evil enough to replace?