Extensible 'resource' loading design critique

[Alan Woodland]

Hi,

I'm currently trying to devise a flexible way for loading a variety of
different resources from a variety of different sources. I've sketched
out the following design in C++, and would appreciate some comments on
the C++ side of things.

- Is my auto_ptr use reasonable? Exception safety is fairly important
here. Any other exception safety related comments?
- Should/Could I have used anything from <algorithm>?
- Is there a better way of doing the copying with the istream in load()?
It feels like a hack like that.

The main aims are to provide a mechanism for allowing 'LoaderModules' to
indicate if they could load something from a given stream. The important
thing that was worrying me though was that one module could interfere
with the istream, and break another module, hence the copying with
stringstreams to isolate them.

Loader and LoaderModule both have one template parameter - P, the
product they load, e.g. Texture/Mesh. In practice the product will be an
interface also, but I've not included any examples here since they're
not really relevant to the question and probably OT anyway.

Eventually I might decide to add other things like caching/sharing etc.,
but for the time being that isn't important so I'll be waiting for it to
flag in a profiler.

#include <string>
#include <istream>
#include <sstream>
#include <fstream>
#include <vector>
#include <exception>
#include <memory>

template <typename P>
class Loader;

template <typename P>
class LoaderModule {
public:
virtual bool accept(std::istream& i) = 0;
virtual std::auto_ptr<P> load(std::istream& i) = 0;

virtual ~LoaderModule() { }

protected:
LoaderModule() {
reg();
}

private:
void reg() {
Loader<P>::getInstance().addModule(this);
}
};

template <typename P>
class Loader {
friend class LoaderModule<P>;
public:
static Loader<P>& getInstance() {
static Loader<P> *inst = NULL;

if (!inst)
inst = new Loader<P>;

return *inst;
}

std::auto_ptr<P> load(const std::string& uri) const {
//in practice I'll make this more flexible here than just files.
std::ifstream in(fn.c_str());
return load(in);
}

std::auto_ptr<P> load(std::istream& s) const {
std:stringstream tmpstream;

if (!(tmpstream << s.rdbuf())) {
// in final version will be more sensible with exceptions here.
throw std::exception();
}

const std::string& copy = tmpstream.str();
for (typename modules_t::const_iterator it = modules.begin(); it
!= modules.end(); ++it) {
std::istringstream i(copy);

if ((*it)->accept(i)) {
std::auto_ptr<P> result = (*it)->load(i);
return result;
}
}

// in final version will also be more sensible with the exception
here.
throw std::exception();
}

private:
void addModule(LoaderModule<P> *m) {
modules.push_back(m);
}

typedef std::vector<LoaderModule<P>*> modules_t;
modules_t modules;

Loader() { }
~Loader() {
for(typename modules_t::iterator it = modules.begin(); it !=
modules.end(); ++it) {
delete *it;
}
}

};

 

[diligent.snail]

To tell the truth, I find some difficulty in following your code. It
seems to me that there are lots of un-needed pointers, with no virtual
behaviour.

One thing comes to mind:

std::auto_ptr<P> result = (*it)->load(i);
return result;

notice that the returned auto_ptr shall delete the pointer when it
goes out of scope, even though it is still available in
Loader::modules vector.

I hope that someone else may provide better help.

Regards.

 

[kasthurirangan.balaji]

Hi,

I'm currently trying to devise a flexible way for loading a variety of
different resources from a variety of different sources. I've sketched
out the following design in C++, and would appreciate some comments on
the C++ side of things.

- Is my auto_ptr use reasonable? Exception safety is fairly important
  here. Any other exception safety related comments?

auto_ptr is good as long as handled well and an evil when not handled
properly. Better and safe is using CountedPtr(Ref:The C++ Standard
Library - Josuttis) or better boost::shared_ptr.

- Should/Could I have used anything from <algorithm>?

always prefer usage of already available,well tested, standardized
code rather than creating one.

- Is there a better way of doing the copying with the istream in load()?
  It feels like a hack like that.

to read from istream, std::copy algorithm could be used along with
istream_iterators.

The main aims are to provide a mechanism for allowing 'LoaderModules' to
indicate if they could load something from a given stream. The important
thing that was worrying me though was that one module could interfere
with the istream, and break another module, hence the copying with
stringstreams to isolate them.

Loader and LoaderModule both have one template parameter - P, the
product they load, e.g. Texture/Mesh. In practice the product will be an
interface also, but I've not included any examples here since they're
not really relevant to the question and probably OT anyway.

Eventually I might decide to add other things like caching/sharing etc.,
but for the time being that isn't important so I'll be waiting for it to
flag in a profiler.

#include <string>
#include <istream>
#include <sstream>
#include <fstream>
#include <vector>
#include <exception>
#include <memory>

template <typename P>
class Loader;

template <typename P>
class LoaderModule {
public:
   virtual bool accept(std::istream& i) = 0;
   virtual std::auto_ptr<P> load(std::istream& i) = 0;

   virtual ~LoaderModule() { }

protected:
   LoaderModule() {
      reg();
   }

private:
   void reg() {
      Loader<P>::getInstance().addModule(this);
   }

};

template <typename P>
class Loader {
   friend class LoaderModule<P>;
public:
   static Loader<P>& getInstance() {
      static Loader<P> *inst = NULL;

since its a static function and a static variable, i guess we can
just use

      if (!inst)
         inst = new Loader<P>;

      return *inst;
   }

   std::auto_ptr<P> load(const std::string& uri) const {
      //in practice I'll make this more flexible here than just files.
      std::ifstream in(fn.c_str());
      return load(in);
   }

   std::auto_ptr<P> load(std::istream& s) const {
      std:stringstream tmpstream;

      if (!(tmpstream << s.rdbuf())) {
         // in final version will be more sensible with exceptions here.
         throw std::exception();
      }

      const std::string& copy = tmpstream.str();

better const std::string &copy(tmpstream.str());

      for (typename modules_t::const_iterator it = modules.begin(); it
!= modules.end(); ++it) {

better for(typename modules_t::const_iterator beg = modules.begin(),
typename modules_t::const_iterator end = modules.end();
beg != end; ++beg) {

         std::istringstream i(copy);

         if ((*it)->accept(i)) {
            std::auto_ptr<P> result = (*it)->load(i);
            return result;
         }
      }

      // in final version will also be more sensible with the exception
here.
      throw std::exception();
   }

private:
   void addModule(LoaderModule<P> *m) {
      modules.push_back(m);
   }

   typedef std::vector<LoaderModule<P>*> modules_t;

   modules_t modules;

   Loader() { }
   ~Loader() {
      for(typename modules_t::iterator it = modules.begin(); it !=
modules.end(); ++it) {
         delete *it;
      }
   }



};- Hide quoted text -

- Show quoted text -

I am not able to provide much to the design end. It looks like the
design is huge. Completely understanding the requirements would put me
in a good position to speak of design.

Thanks,
Balaji.