6

I am trying to create a constructor to load a resource from any istream given to it. I cannot seem to figure out the best way to pass the istream parameter into a constructor.

Loader::Loader(istream stream);

This one is obviosly bad due to object slicing, so no option.

Loader::Loader(istream& stream);

This is what I am using now and seems fairly alright. It has one significant issue though - you can't give it a temporary since temporaries cannot bind to non-const references! For example, the following won't work:

Container():
  mLoader(ifstream("path/file.txt", ios::binary)
{
}

This is rather a limitation since I am now forced to store the ifstream as a member variable of Container just to extend its lifetime.

Since the problem is with non-const references, one could have though of this:

Loader::Loader(const istream& stream);

But since .seek() etc are non-const, this is not an option either...

So, how can this problem be solved in a neat way?

Tobias
  • 924
  • 9
  • 23

3 Answers3

7

if your compiler is c++11 or better you can simply provide a version of the constructor that takes the istream as an r-value reference:

void Loader::Loader(std::istream&& is)

quick example:

#include <iostream>
#include <iomanip>
#include <string>
#include <sstream>

struct Loader
{
    Loader(std::istream& is)
    {
        read(is);
    }

    Loader(std::istream&& is)
    {
        read(is);
    }

    void read(std::istream& is)
    {
        is >> std::quoted(x);
        is >> std::quoted(y);
    }

    std::string x, y;
};

std::ostream& operator<<(std::ostream& os, const Loader& l)
{
    os << "x = " << l.x;
    os << ", y = " << l.y;
    return os;
}


auto main() -> int
{
    using namespace std;

    Loader l(istringstream(R"text("donkey" "horse")text"));
    cout << l << endl;

    istringstream not_temp(R"text("apple" "banana")text");
    Loader l2(not_temp);
    cout << l2 << endl;

    return 0;
}

expected output:

x = donkey, y = horse
x = apple, y = banana
Richard Hodges
  • 68,278
  • 7
  • 90
  • 142
  • This makes sense IMO. By providing both a Loader::Loader(std::istream&& is) and Loader::Loader(std::istream& is), and have the former call the latter, you can use it in both ways. Thanks! – Tobias Dec 09 '15 at 14:18
  • I don't see the point in defining such a constructor for each class where it's desired. As I see it, a single centralized solution such as a `temp_ref` function is less code (linearly in the number of classes), and more clear. – Cheers and hth. - Alf Dec 09 '15 at 14:42
  • I understand your position(s). Less code (linearly in the number of cases) I agree with. More clear, I would probably take issue with. – Richard Hodges Dec 09 '15 at 14:44
2

The example

Container():
  mLoader(ifstream("path/file.txt", ios::binary)
{
}

… is ill-conceived, because the lifetime of the temporary would be just the constructor call. Then you'd be left with a dangling reference. Using that reference would be Undefined Behavior.

However, technically, in order to be able to pass a temporary to a reference-to-non-const formal argument, just define a little helper like this:

template< class Type >
auto temp_ref( Type&& o )
    -> Type&
{ return o; }

Then you can call e.g. foo( temp_ref( Whatever() ) ), but just keep in mind that the lifetime of that temporary is the full-expression that it occurs in.

Cheers and hth. - Alf
  • 142,714
  • 15
  • 209
  • 331
-2

Passing a pointer to a newly created object would work. 

Loader(istream * pstream)
{
    try
    {
        // ......

        delete pstream;
        throw; 
    }
    catch(...)
    {    
        delete pstream;                  
    }
}

Container():
mLoader(new ifstream("path/file.txt", ios::binary))
{
}
user93353
  • 13,733
  • 8
  • 60
  • 122
  • 1
    This is a hideous resource leak waiting to happen! – Richard Hodges Dec 09 '15 at 14:25
  • More like crash when you pass the address of a stack allocated object. – Tobias Dec 09 '15 at 14:26
  • 1
    @Tobias AND a forced redundant memory allocation. – Richard Hodges Dec 09 '15 at 14:27
  • @RichardHodges - how is this a memory leak waiting to happen? – user93353 Dec 09 '15 at 15:43
  • @user93353 if an exception is thrown in the constructor you'll be left with an orphaned istream on the heap - worse, its file handle will still be open. – Richard Hodges Dec 09 '15 at 15:47
  • right, so there are a few problems remaining: 1) the exception is not propagated so you will end up with a half-constructed object. 2) if the exception happens during the evaluation of the initialiser list or the default initialisation of member variables, your try/catch won't catch it so you're back to square one with a resource leak. 3) you still have a redundant memory allocation 4) you you rely on the caller not passing you the address of a static, global or stack-based istream. For these (and other) reasons, good c++ design avoids the use of pointers in interfaces. – Richard Hodges Dec 09 '15 at 16:01
  • @RichardHodges - 1. was planning to put a rethrow, but forgot about it - added it now. 2. There was no initialiser list - hence did not use a function try block. 3. He wanted a temp - there is no other way to have a temp. – user93353 Dec 10 '15 at 03:32
  • There is an implicit initialiser list. Any member that's not mentioned in the initialiser list is default constructed. If any of these default constructors throw, you have a resource leak. The way to pass a temporary is to pass a temporary, which either of the other two answers allow. If you _really_ insist on passing a pointer, the only way to make it safe is to pass a unique_ptr (or unique_ptr const ref). But why would you when you can simply pass a temporary, which is succinct, simple, supported by the language, unambiguous, safe and requires no special knowledge at the call site? – Richard Hodges Dec 10 '15 at 07:31