meven added a comment.
In D25039#557004 <https://phabricator.kde.org/D25039#557004>, @anthonyfieroni wrote: > Not using references is not a big problem with QString nor QUrl since they are reference counting objects, say if you don't change their content they are immutable, so > > const QString s = other; // it's fine > void func(QString s) > { > const QString o = s; // use o instead of s is also fine, using plain s is fine too, if you don't touch mutability > ... > } > Using cont &, you still save some reference counting overhead and allow the compiler to make potential better optimization. In your example you'd also save the boilerplate needed just to have a variable const. This also makes API clearer about what to expect. INLINE COMMENTS > kossebau wrote in ftp.cpp:1378 > This here makes the code fragile and more confusing. What is the difference > between `filename` & `search`? Why is `filename` not const? Can it ever > change? Would that be fine for `search` to change as well? > So while seemingly a correct optimization, as `filename` seems not passed to > any method modifying it, the resulting code is very strange to a human > reader, the intent behind to have `search` being a const reference to > `filename` seems mysterious. > > Without having understood the code, I would simply also make `filename` a > const variable, and add a hint why search is a const reference only (hinting > this is for optimization). > Though actually `search` could be possibly be removed and checked what the > purpose of the asserts have been and merge this with the code upfront. But > outside of scope here, so leaving just a TODO for the next person should be > fine. One can still be the next person oneself :) I believe search can be removed in fact, fileName can be used interchangeably. That's the const beauty it makes this kind of issue visisble. But this patch is about fixing atomic warnings, not fixing code around those warnings. This can be fixed in a subsequent diff, thanks for pointing it out. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D25039 To: meven, #frameworks, dfaure Cc: anthonyfieroni, kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
