Merge lp://staging/~aacid/thumbnailer/fast_string_concatenation into lp://staging/thumbnailer/devel

FAILED: Continuous integration, rev:287
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/485/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/295/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/300/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/294/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/485/rebuild

PASSED: Continuous integration, rev:288
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/486/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/301
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/301/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/295

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/486/rebuild

I'm feeling a bit ambivalent about this change now. The original change picked up by the tool was:

- string inpath = (artist_ + "_" + album_).toStdString() + "_" + suffix;
+ string inpath = QString(artist_ + "_" + album_).toStdString() + "_" + suffix;

Fair enough, it's a little bit more efficient this way. But, in practice, it doesn't make one iota of difference because the cost of the string catenation is infinitesimal compared to the overall cost of the operation. (We are about to do a client-server round trip.)

And now we have all the other changes where the cast to QString is needed. I'm wondering whether that really is worth it. We have more cryptic code in quite a few places now, in return for a performance improvement that is immeasurably small. Is this the right trade-off?

It is not always about speed but also less allocations happening meaning the memory gets less fragmented which is as well interesting for long lived apps.

As for wheter

- string inpath = (artist_ + "_" + album_).toStdString() + "_" + suffix;
+ string inpath = QString(artist_ + "_" + album_).toStdString() + "_" + suffix;

is more readable or not i guess it's up to you, but it adds some more explicitness, meaning readers that are not the ones that immediately wrote the code know that the toStdString is being called over a QString and don't have to guess the types of artist_ and album_ check if they are the same and then check the operator + with "_" still gives the same type.

Well, sure, I agree with the memory fragmentation in principle. But, let's face it, this just isn't going to make any noticeable difference. There are dozens (if not more) allocations elsewhere for each request.

What I find worrying here is are all the other changes, such as this one:

- throw parser.errorText() + "\n\n" + parser.helpText();
+ throw QString(parser.errorText() + "\n\n" + parser.helpText());

Adding the -DQT_USE_QSTRINGBUILDER caused previously working code to fail because, suddenly, what is thrown is a string builder instead of a string. I can't say I like this magic all that much. The same single expression (a + b) can return two different types, depending on whether the symbol is defined or not.

A less intrusive change would be to not do all the casts where the exceptions are thrown and instead add a catch handler for string builder in main? But, even then, this leaves a bad taste in my mouth. Changing the type of an expression via a preprocessor symbol is dangerously close to preprocessor abuse, IMO.

Michi doesn't like the change