[KPhotoAlbum] call for review: misc patches

[Shawn Willden]

On Friday 18 May 2007 09:42:18 am Jan Kundrát wrote:

> Thanks a lot for an excellent review, updated patch is at the old location.

Most welcome.  I really enjoy these little intricacies of C++.  Probably I'm 
crazy :)

> > 	QString fileName = files.first();
>
> Changed to:
>
> 	const QString fileName = files.first();
>
> I hope this is enough. I'm sorry, but I'm not as familiar with C++ as
> I'd like to be.

I don't think you need to worry about this particular copy, because the list 
only contains a single element.  However, just for completeness I should 
point out that making the fileName const, while not a bad idea, doesn't 
prevent first() from making a copy of the list.

There are two implementations of QValueList<T>::first():

T& first() { 
	QT_CHECK_INVALID_LIST_ELEMENT; 
	detach(); 
	return sh->node->next->data; 
}

const T& first() const {
	QT_CHECK_INVALID_LIST_ELEMENT; 
	return sh->node->next->data;
}

(I pulled these from qvaluelist.h and reformatted them for readability).

The non-const version calls detach(), which checks the reference count and if 
it's greater than one makes a new copy of the list so that modifications are 
not seen by other code holding references.

You want to call the const version, obviously, but C++ doesn't consider the 
type of the object you're assigning the result to when deciding which of the 
overloaded functions to call.  It uses only the method parameters to make 
that decision, including the implicit "this" parameter, which is the only 
parameter to first().

So, if you want to avoid the copy, you need to make "files" const, so that the 
compiler can use the const version of first().  One way to do this is:

	const QStringList& constFiles = files;
	QString fileName = constFiles.first();

A more compact but perhaps less readable way is:

	QString fileName = ((const QStringList&)files).first();

Should you also make fileName const?  It doesn't matter, really, except on the 
general principle that anything you don't intend to modify should be const, 
mainly to catch errors.  The principle is usually called "const correctness" 
and it serves to ensure that you don't inadvertently modify anything you 
didn't mean to modify.  It's a good idea, IMO.

	Shawn.