Alternatives to raw pointers and design suggestions needed

[veroslav]

Hi,

I have the following code:

BinaryEncodedDictionary* BinaryMetaDataDecoder::parse(QIODevice *sourceDevice) {
	qDebug() << "Received a QIODevice to parse from!";
 
    if(sourceDevice->peek(1) != "d") {
        qDebug() << "Input stream contains an invalid input file";
		return 0;
    }
    else {
        qDebug() << "Found a dictionary, valid file";
		return parseDictionary(sourceDevice);
    }	
}
 
BinaryEncodedDictionary* BinaryMetaDataDecoder::parseDictionary(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedDictionary;
}
 
BinaryEncodedInteger* BinaryMetaDataDecoder::parseInteger(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedInteger;
}
 
BinaryEncodedString* BinaryMetaDataDecoder::parseString(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedString;
}
 
BinaryEncodedList* BinaryMetaDataDecoder::parseList(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedList;
}

BinaryEncodedDictionary* BinaryMetaDataDecoder::parse(QIODevice *sourceDevice) {
	qDebug() << "Received a QIODevice to parse from!";

    if(sourceDevice->peek(1) != "d") {
        qDebug() << "Input stream contains an invalid input file";
		return 0;
    }
    else {
        qDebug() << "Found a dictionary, valid file";
		return parseDictionary(sourceDevice);
    }	
}

BinaryEncodedDictionary* BinaryMetaDataDecoder::parseDictionary(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedDictionary;
}

BinaryEncodedInteger* BinaryMetaDataDecoder::parseInteger(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedInteger;
}

BinaryEncodedString* BinaryMetaDataDecoder::parseString(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedString;
}

BinaryEncodedList* BinaryMetaDataDecoder::parseList(QIODevice *stream) {
	//... Implementation will follow here
	return new BinaryEncodedList;
}

To copy to clipboard, switch view to plain text mode 

All of the classes above are plain old C++ classes (not inheriting from QObject) and part of
the following class hierarchy:

Pure virtual (super) class: BinaryEncodedAbstractType
Child (inheriting classes): BinaryEncodedDictionary, BinaryEncodedInteger, BinaryEncodedString, BinaryEncodedList

BinaryEncodedDictionary is implemented like this:

//binaryencodeddictionary.h
class BinaryEncodedDictionary : public BinaryEncodedAbstractType {
public:
	BinaryEncodedDictionary();
	//TODO: Delete pointers contained by hashMap
	~BinaryEncodedDictionary();
private:
	QHash<BinaryEncodedString*, BinaryEncodedAbstractType*> hashMap;
};
 
//binaryencodeddictionary.cpp
BinaryEncodedDictionary::BinaryEncodedDictionary() {
	hashMap = new QHash<BinaryEncodedString*, BinaryEncodedAbstractType*>();	
}
 
BinaryEncodedDictionary::~BinaryEncodedDictionary() {
	//TODO: We need to remove dynamically allocated values (needed for polymorphism)
}

//binaryencodeddictionary.h
class BinaryEncodedDictionary : public BinaryEncodedAbstractType {
public:
	BinaryEncodedDictionary();
	//TODO: Delete pointers contained by hashMap
	~BinaryEncodedDictionary();
private:
	QHash<BinaryEncodedString*, BinaryEncodedAbstractType*> hashMap;
};

//binaryencodeddictionary.cpp
BinaryEncodedDictionary::BinaryEncodedDictionary() {
	hashMap = new QHash<BinaryEncodedString*, BinaryEncodedAbstractType*>();	
}

BinaryEncodedDictionary::~BinaryEncodedDictionary() {
	//TODO: We need to remove dynamically allocated values (needed for polymorphism)
}

To copy to clipboard, switch view to plain text mode 

The hashMap member above contains the heap-allocated return values from calls to parse*() functions as keys
and values. Without going further into implementation details (if not needed), my main concern is the
deletion of objects allocated by the parse*() functions. The returned BinaryEncodedDictionary from parse()
function contains the pointers to these objects through the hashMap member variable.

I am trying to make sure that the objects are de-allocated when BinaryEncodedDictionary goes out of scope eventually,
and at the same time, I would like to make use of QScopedPointer and/or QSharedPointer in order not to have
to deal with manually freeing the memory with delete.

How could I use above in my code? I am planing to return QScopedPointers instead of raw pointers as the return
values from the functions above, but will it be enough or do I have to do anything more? Any other suggestions?

I am very thankfull to any kind of desing improvement suggestions as well.

Thank you in advance.

Kind Regards,
Veroslav

[Coises]

Instead of making hashMap a pointer that you initialize to a new QHash<...>, you can almost certainly just make it a QHash<...>. (I’m assuming the declaration of hashMap you gave is missing an *, since you assign a new QHash to it.) Note that QHash is one of Qt’s implicitly shared classes. Because of that, there is no need to pass around QHash pointers to avoid copying lots of data; you can pass QHash values instead. (That might or might not make much difference in your design.)

I could be missing something, but I don’t think QScopedPointer is going to help you here. Your various parseXxxxx methods are essentially factories; it wouldn’t make any sense (ever, I think) to return a QScopedPointer from such a function, though you could assign the return value to a QScopedPointer. If I’m reading this right, though, that wouldn’t make any sense here either, because the lifetime of those objects isn’t controlled by the scope in which the factory functions are called, but by the lifetime of the top-level BinaryEncodedDictionary.

I don’t think you can avoid having to have the destructor for a BinaryEncodedDictionary clean up the objects to which its QHash’s keys and values point. (You can’t use a QScopedPointer for the key or value types because it isn’t an assignable data type.) QSharedPointer is a possibility, though it’s not clear to me that it would be better than just writing the appropriate destructor code. I don’t think you need “smart pointers” for this; they would just hide the deletes that must be done either way behind an extra layer of classes to understand.

A discussion of Qt pointer wrappers that might be helpful, if you do choose to use them:
Count with me: how many smart pointer classes does Qt have?

[veroslav]

Coises,

first let me thank you for your thorough and consise answer. There were quite a few good tips in there
and I realize that I need to do more reading on the topic, especially on Qt containers and pointer/memory
management.

The main reason I created this thread was to find out whether I should use the smart pointers at all (I was
trying to avoid manually calling delete as I read you should avoid it if possible). I wanted to check that
it was possible to avoid it in my case and you confirmed to me what I suspected from the beginning: delete
may just be more appropriate to use in my case.

I will need to read your reply few more times to take it all in, and then I will give it a go and try to
come up with some good implementation. Now I have something to bite my teeth in!

Again, thank you very much for giving me some tips to get going again. I will post back in case I hit problems
again. It's fun learning again!

Kind Regards,
Veroslav