[KPhotoAlbum] call for review: misc patches

[Shawn Willden]

On Wednesday 16 May 2007 03:54:16 pm Jan Kundrát wrote:
> I'd love if someone familiar with C++ can check if I don't leak memory
> somewhere because I feel somehow uncomfortable when overwriting one
> QStringList with another...

I don't see any potential memory leaks.  QStringList, QString, etc. handle all 
of the memory management automagically.  Most all of the Qt classes use 
reference counting and copy-on-write semantics so copying is fast and cleanup 
is automatic.

I do have one comment about the code, however:  It makes an unnecessary copy 
of the list returned by currentScope().  The line:

	files = DB::ImageDB::instance()->currentScope(  true );

actually does not make a copy of the list data, it only increments the 
reference count.  However, the expression:

	QStringList::Iterator it = files.begin()

*does* make an actual copy of the data, perhaps copying the entire database of 
file names.  Getting a non-const iterator qualifies as a "write" operation 
and so invokes the "copy" part of "copy-on-write".  Even though you don't 
modify anything through that iterator, the QStringList implementation doesn't 
know that, so on the grounds that you must intend to make modifications 
(otherwise you'd have used a const iterator), it "detaches" the list.

In addition to that copy, the line:

	QString fileName = files.first();

also assumes a write (you could modify the reference returned by first()) and 
therefore creates a copy.  Of course, that list only contains a single 
element.

To avoid copying the list returned by currentScope(), use constBegin(), rather 
than begin(), and you'll have to assign it to a const_iterator.  The other 
use of the list is when it is passed to ViewerWidget::load(), but it's passed 
as a const reference, so there's no possible write to it, and no need for a 
copy to be made.

	Shawn.