D26130: Simplify return logic

[Kevin Ottens]

ervin added a comment.


  In D26130#582465 <https://phabricator.kde.org/D26130#582465>, @patrickelectric wrote:
  
  > In D26130#582447 <https://phabricator.kde.org/D26130#582447>, @ervin wrote:
  >
  > > In D26130#582391 <https://phabricator.kde.org/D26130#582391>, @tcanabrava wrote:
  > >
  > > > In D26130#582343 <https://phabricator.kde.org/D26130#582343>, @ervin wrote:
  > > >
  > > > > This is pointless, most compilers would do NRVO (+ move assign) here... which would be neatly obliterated by the ternary operator. This is a pessimisation you're proposing here, not an optimization.
  > > >
  > > >
  > > > I don't know what you are talking about, this is using return time optimization. have you tested the code or just assumed that ternaries will not do NRVO?
  > >
  > >
  > > OK, let me try again.
  > >
  > > I did benchmark old and new before my first reply (and I wonder why I wrote pessimisation previously, I think I got carried away a bit, apologies). What I meant was that: if and when there is a small measurable gain, I don't think it is as dramatic as you make it sound. The point I was trying to make was that since the ternary will prevent NRVO, you're trading one return optimization (NRVO) for another (RVO). In other words, the same amount of objects will be involved. It's far from the "there was no return optimization now there's one" that I understood you're trumping. Thus the gain between the two versions when there is, is unlikely to come from RVO. It is mostly about avoiding the move assignment in the newer version (roughly a couple pointers copy).
  > >
  > > Now, the **really** interesting bit you did is wrapping "d->" in QStringLiteral, that's what gives a large gain for the dpointer case (when I did the benchmarking I did it with QStringLiteral in both versions, otherwise I knew it was just unfair).
  > >
  > > Obviously I'd be totally cool with just a QStringLiteral change since it would have real value (divides time by three roughly, this is definitely a large gain).
  >
  >
  > If the patch is cleaner, has less code, make it in general more readable, easier to maintain and performance is not a issue at all in this simple function, why does matter it if it's optimized or how is optimized ? This code does not need to be super optimized and the actual patch that @tcanabrava  proposes is a better code in general comparable with the old one.
  >  I don't see why this discussion is happening and why, to me performance is a problem if it is a real problem, otherwise no and this discussion is pointless.
  
  
  Ah! Right: I also take into account the commit log. And in that case it is wrong: this is barely an optimization except for the QStringLiteral. It should say so.
  
  As for "cleaner" and "more readable", this is very debatable and subjective. That's why I consider the rest (beyond the QStringLiteral introduction this is where the value of that patch is: but it should say so) to be mostly about personal style which is not a good reason to start a patch in the first place.
  
  Am I being pedantic? Yes. It's motivated by the fact that we're touching a very critical and old component in our frameworks. It needs to be handled with lots of extra care while the changes proposed are in part and unwillingly justifying personal preferences under other motives and that gives me the creeps.
  
  Before this gets the wrong spin because the internet and text based communication lead to that: I'm not accusing Tomaz of being evil or trying to disguise his motives. I like the author of those patches and I don't think this is what's going on at all. But, I'm trying to push for energy to be channeled and focused at the right places in the future patches.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D26130

To: tcanabrava, patrickelectric, ervin
Cc: ervin, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191223/db478e8c/attachment.html>