The method InlineBox::setHasBadParent is only used in debug, so don't provide this function in release.
Created attachment 140436 [details] Patch
Comment on attachment 140436 [details] Patch I'm not sure this helps much. I guess all callers were already wrapped behind NDEBUG flags?
Hi Eric, (In reply to comment #2) > (From update of attachment 140436 [details]) > I'm not sure this helps much. I guess all callers were already wrapped behind NDEBUG flags? Not claiming a huge speedup :) But why provide a no-op method in release that is not called? Yes the one caller is wrapped in NDEBUG. Cheers, Rob.
(In reply to comment #3) > Hi Eric, > > (In reply to comment #2) > > (From update of attachment 140436 [details] [details]) > > I'm not sure this helps much. I guess all callers were already wrapped behind NDEBUG flags? > > Not claiming a huge speedup :) But why provide a no-op method in release that is not called? Yes the one caller is wrapped in NDEBUG. > Cheers, > > Rob. I think I should have said, method with empty body.
Comment on attachment 140436 [details] Patch Clearing flags on attachment: 140436 Committed r116253: <http://trac.webkit.org/changeset/116253>
All reviewed patches have been landed. Closing bug.
Why make this change? The other way can be nicer since you don’t have to #ifdef every call site if it’s the only thing that would require an ifdef. Also, if we’re not doing the NDEBUG version, then I see no point in making this inline.
Hi Darin, (In reply to comment #7) > Why make this change? The other way can be nicer since you don’t have to #ifdef every call site if it’s the only thing that would require an ifdef. Right, but at the moment there is only one call site, and it is in a NDEBUG section. > Also, if we’re not doing the NDEBUG version, then I see no point in making this inline. I am not even thinking about inline, the goal of my patch is to simply remove the method for release builds , since it is only useful in debug builds. Basically it is the way I would have done this code myself. I don't know if the release binary gets smaller or the compiler is clever enough to not generate code for this unused function (in release). Maybe the inline is enough of a clue for the compiler to do that, I just found my way clearer. Basically I happened to have this change in my git tree and I needed to get it out of the way :) If you feel strongly for the old approach, a revert is possible of course. Cheers, Rob.
(In reply to comment #8) > I don't know if the release binary gets smaller I do know. It does not get smaller. > the compiler is clever enough to not generate code for this unused function (in release) It is. > I just found my way clearer. OK. We disagree on this. I like the old way. I don’t think we should be changing things like this, especially in code that we’re not otherwise changing. I think fewer #ifdefs are better for code readability. Functions that are empty inlines in release builds are a technique used all over the place in WebCore. So there should be no doubt that the technique works unless we see an actual problem. I am not going to push you to change this in this case, but lets please not head in this direction in the future.
Hi Darin, (In reply to comment #9) > (In reply to comment #8) > > I don't know if the release binary gets smaller > > I do know. It does not get smaller. > > > the compiler is clever enough to not generate code for this unused function (in release) > > It is. > > > I just found my way clearer. > > OK. We disagree on this. I like the old way. > > I don’t think we should be changing things like this, especially in code that we’re not otherwise changing. I think fewer #ifdefs are better for code readability. > > Functions that are empty inlines in release builds are a technique used all over the place in WebCore. So there should be no doubt that the technique works unless we see an actual problem. > > I am not going to push you to change this in this case, but lets please not head in this direction in the future. Ok, I did not know this, I'll indeed not do changes like that in the future.