Maybe own a resource: is this a good design?

[Steven T. Hatton]

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.

 

[Tobias Blomkvist]

Steven T. Hatton sade:

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.

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

 

[=?ISO-8859-15?Q?Juli=E1n?= Albo]

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?

I doubt that "I don't like this" will be considered a contribution.

 

[Steven T. Hatton]

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

I was trying to determine if my observations were reasonable and valid.