Maybe own a resource: is this a good design?

Discussion in 'C++' started by Steven T. Hatton, Jul 31, 2005.

  1. I happened across this little library for playing with ELF files.

    http://elfio.sourceforge.net/

    It's written in C++ which makes it more attractive than most packages like
    this, which are written in C. The design seems reasonably coherent.
    Nonetheless, there are a few aspects which I decided to see if I could
    change. One of these was the fact that it passes pointers to istreams
    rather than references. It also does stuff like void** ppObj, which always
    makes me think someone's trying to pull one over on me. It's pretty clear
    to me that it's not making extensive, and direct use of RAII, os I decied
    to see if I could change that.

    While I was digging through the uses of std::istream trying to iliminate
    pointers, I cam across this

    ELFI::ELFI()
    {
    m_nRefCnt = 1;
    m_nFileOffset = 0;
    m_bOwnStream = false;
    m_bInitialized = false;
    std::fill_n( reinterpret_cast<char*>( &m_header ), sizeof( m_header ),
    '\0' );
    }

    // all of this distructory stuff make me uncomfortable, and I'm going to try
    // to find a cleaner way to do it. Not that it's terrible, or anything.

    ELFI::~ELFI()
    {
    std::vector<const IELFISection*>::const_iterator it;
    for ( it = m_sections.begin(); it != m_sections.end(); ++it ) {
    delete const_cast<IELFISection*>( *it );
    }

    std::vector<const IELFISegment*>::const_iterator it1;
    for ( it1 = m_segments.begin(); it1 != m_segments.end(); ++it1 ) {
    delete const_cast<IELFISegment*>( *it1 );
    }

    //But this one stood out:
    //vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
    if ( m_bOwnStream ) {
    ((std::ifstream*)m_pStream)->close();
    delete m_pStream;
    }
    }
    //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    The things I don't like about it are that:

    * it makes the class a "maybe file handle".
    * the way it was implemented, it requires the streams be held by pointers,
    * it's not using RAII "correctly", in the sense that it doesn't open the
    file in the constructor, and it doesn't use the delete to close the file,
    * It seems to me there should, at a minimum, some kind of smart pointer,
    * If an instance of this class can own a stream, or it could just as easliy
    hold a stream owned by another instance, it seems possible that someone
    might deleate a busy stream.

    There is some referencing counting going on for the whole ELFI class, which
    may be sufficient. The desigin doesn't seem /that/ bad, it just "feels"
    wrong. Any other opinions on this?

    I acknowledge that I am evaluating it without much knowledge of the larger
    context. In some ways I believe that is a correct approach. In otherways,
    it's probably very dangerous. Part of the reason I'm stopping to analyze
    it before I continue to examine the program is because I want to test how
    valid my observations are from this limited perspective.
     
    Steven T. Hatton, Jul 31, 2005
    #1
    1. Advertisements

  2. Steven T. Hatton sade:
    If the design is flawed by your standards, then why not join the project
    and do a contribution, or at least direct your concerns to the developers?

    Tobias
     
    Tobias Blomkvist, Jul 31, 2005
    #2
    1. Advertisements

  3. I doubt that "I don't like this" will be considered a contribution.
     
    =?ISO-8859-15?Q?Juli=E1n?= Albo, Jul 31, 2005
    #3
  4. I was trying to determine if my observations were reasonable and valid.
     
    Steven T. Hatton, Aug 1, 2005
    #4
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.