[bt-devel] Programming Techniques

[David White]

hi Joachim,

Thanks for your welcome; answers to your questions are below.

On Sat, 2002-03-09 at 03:08, Joachim Ansorg wrote:
> 
> At least I know that we should allocate on the stack, because we can't forget 
> to delete the object :)

Precisely. However, looking through the code I noticed some places where
this was not done, when it could have been, e.g.
CStyleList::openStyleEditor has code that looks like this:

CStyleEditorDlg* dlg = new CStyleEditorDialog(style,this);
dlg->exec();
delete dlg;

It should instead read:

CStyleEditorDlg dlg(style,this);
dlg.exec();

this is simpler, faster and easier to maintain. If you want to ensure
that an object is deleted by a certain time, just add an extra scope
(i.e. pair of braces) around it.

(Exception: Some gui libraries require that certain objects be allocated
on the heap. This is not common, but if it is the case with Qt then this
would be an exception)

> But often we have code like
> 	CSwordKey* key = CSwordKey::createInstance(module);
> Is there a way to do this on the stack?

No, because Sword (apparently) creates it on the heap inside
CSwordKey::createInstance and then returns the key to you. I am assuming
that the reason Sword does this, rather then return the object by value,
and avoid dynamic allocation is that these key objects exhibit
polymorphic behaviour.

In this case, this is where my scoped_resource stuff comes in. However,
I'm not sure if the way Sword does this is the best way to do it. I
would have tended to think that keys should be concrete types, but not
knowing the design decisions etc I can't really comment. So of course,
you would change that code to:

const util::scoped_ptr<CSwordKey>
key(CSwordKey::createInstance(module));

and then remove the deletion down the bottom.

> 
> And at the moment we use much code like 
> 	CSwordVerseKey* vk = dynamic_cast<CSwordVerseKey*>( 
> CSwordKey::createInstance(module) ); 
> 
> Is there a way to get rid of the cast? Sorry for asking all these (probably 
> dumb) questions, but I don't know C++ well enough.

Don't worry, your questions are very sensible and good questions.

I assume CSwordVerseKey is a class (directly or indirectly) derived from
CSwordKey. Now, firstly the key class hierarchy should do its best to
use polymorphism so that you don't have to get a pointer to the derived
class. If you do have to get a pointer to the derived class, in my
opinion that is a sign of a potential design flaw in the CSwordKey
hierarchy.

Now, there are two situations in this case:

(1) you know that the object returned by createInstance will be of type
CSwordVerseKey
(2) you think that the object returned by createInstance will be of type
CSwordVerseKey but you're not sure, and you want to be able to recover
if it's not

If case (1) is true, then use a static_cast instead of a dynamic_cast
If case (2) is true, then use a dynamic_cast and check if the result is
NULL. If the result is NULL, it means that the object is not really a
CSwordVerseKey object, and is some other kind of object (still an object
derived from CSwordKey though, unless someone has done something evil).

As far as I can see, what you are doing at the moment is using a
dynamic_cast and then not checking if the result is NULL. You should
never do this - if you are sure enough that the object will be of type
CSwordVerseKey, then use a static_cast.

However, since we can never be sure of anything in programming, it would
probably be a good idea to write a function like this:

template<typename To,typename From>
To static_downcast(From p) {
#ifdef debug
	To to = dynamic_cast<To>(p);
	assert(to != NULL);
	return to;
#else
	return static_cast<To>(p);
#endif
}

and use that to do down casting of the kind you describe - this gets the
useful error handling of a dynamic_cast in debug mode, and the speed of
a static_cast in release mode. You would use it just like the other
casts.

I may write more about the best way to do casting later.

> 
> > BibleTime uses the new operator, to start with, and the C++ standard
> > says that if new fails, it will throw an exception. It's just about
> > impossible to program in C++ without touching exceptions. Even if you
> > could get around the exception problem, you would still have problem 1,
> > which is that this is difficult to maintain, because you have to
> > remember to delete ob.
> 
> AFAIK Qt overloads the new operator, but I'm not sure about this. And I never 
> used exceptions, so I can't judge about this :)

well, using exceptions is A Good Thing. Maybe we should discuss
BibleTime and exceptions sometime in the future :)

> 
> > struct close_file { void operator()(int fd) const { close(fd); } };
> >
> > {
> >    scoped_resource<int,close_file> file(open("file.txt",O_RDONLY));
> >    ...use file...
> > } //file is automatically closed
> >
> > I understand this might be all quite confusing, but once you understand
> > it it's really cool. Please let me know if you don't understand
> > anything.
> 
> We do at almost all places use QFile on the stack, which closes itself (I 
> hope :)

Yes, I assumed that, however I was just giving an example of how you
could use my class to manage a resource other than memory, and file
handles was the first thing that sprang to mind :)

> 
> 
> While converting some of the code I run into trouble (function 
> CPrintItem::moduleText):
> 	CSwordKey* startKey =  CSwordKey::createInstance(m_module);
> 	CSwordKey* stopKey = CSwordKey::createInstance(m_module);
> 	if (.. left out here ..) {
> 		CSwordVerseKey* vk_start = dynamic_cast<CSwordVerseKey*>(startKey);
> 		CSwordVerseKey* vk_stop = dynamic_cast<CSwordVerseKey*>(stopKey);			
> 	}		
> 	delete startKey;
> 	delete stopKey;					
> 
> If I use 
> 		util::scoped_ptr<CSwordKey> startKey( CSwordKey::createInstance(m_module) );
> 		util::scoped_ptr<CSwordKey> stopKey( CSwordKey::createInstance(m_module) );
> the casts 
> 		CSwordVerseKey* vk_start = dynamic_cast<CSwordVerseKey*>(startKey);
> won't work. Is the casting inelegant C++? Any way to make this work with 
> scoped_ptr?
> Maybe this is related to the comments at the beginning of this mail.

ok, I put an implicit conversion in scoped_resource, to convert to the
type it is wrapping, however this conversion won't be found by the
compiler in cases like this. The way to do this would be like this:

CSwordVerseKey* vk_start =
dynamic_cast<CSwordVerseKey*>(static_cast<CSwordKey*>(startKey));

to explicitly convert to type CSwordKey* before converting to
CSwordVerseKey*.

However, I know you are not going to like me insisting that you do it
this way. :) As such, I have added a member function called get() which
will explicitly convert to the pointer if needed. So when doing things
like dynamic_casts, do it like this:


CSwordVerseKey* vk_start =
dynamic_cast<CSwordVerseKey*>(startKey.get());

Usually you shouldn't need to do it like this, but for things like casts
you will.

> 
> 
> One last question: How do we manage things if the deklaration of a member is 
> in the header file and the new statement is in the .cpp file? For example 
> CHTMLWidget::m_toolTip.
> Help is appreciated :)

scoped_resource is not designed to be used for class members. It's only
designed for when a resource is created and destroyed within a single
scope. scoped_resource is a simple class, and prohibits things like
copying, so you will find it impossible to do things like this with it.
(If you think about it, if shared_resource got copied, since it frees
the resource when it gets deleted, if it were copied, it would be
deleted twice, and hence free the resource twice - not something we
want!)

A resource that is a class member, and is released in the destructor of
the class, is not nearly so much a problem as objects that are acquired
and then released within a single scope. This is because the language
guarantees calling of the class's destructor, and if an exception is
thrown in a destructor, you have far too many problems to start worrying
about resources not being released :)

However, I may add a more flexible resource management wrapper,
smart_resource, which uses reference counting to allow it to offer
copying, and hence be used in more situations.

Blessings,

David.

> 
> Thank you for your help! I'm glad to have someone with excellent C++ 
> knowledge in the team :)
> Joachim
>