85748 2012-05-06 14:36:35 -0700 InlineBox::setHasBadParent should be debug only 2012-05-07 12:27:13 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 rwlbuis rwlbuis darin eric webkit.review.bot oldest_to_newest 617170 0 rwlbuis 2012-05-06 14:36:35 -0700 The method InlineBox::setHasBadParent is only used in debug, so don't provide this function in release. 617173 1 140436 rwlbuis 2012-05-06 14:41:00 -0700 Created attachment 140436 Patch 617178 2 140436 eric 2012-05-06 14:49:45 -0700 Comment on attachment 140436 Patch I'm not sure this helps much. I guess all callers were already wrapped behind NDEBUG flags? 617179 3 rwlbuis 2012-05-06 14:54:43 -0700 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. 617180 4 rwlbuis 2012-05-06 14:55:12 -0700 (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. 617240 5 140436 webkit.review.bot 2012-05-06 17:44:28 -0700 Comment on attachment 140436 Patch Clearing flags on attachment: 140436 Committed r116253: <http://trac.webkit.org/changeset/116253> 617241 6 webkit.review.bot 2012-05-06 17:44:33 -0700 All reviewed patches have been landed. Closing bug. 617249 7 darin 2012-05-06 18:11:15 -0700 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. 617262 8 rwlbuis 2012-05-06 18:49:00 -0700 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. 617685 9 darin 2012-05-07 10:25:30 -0700 (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. 617826 10 rwlbuis 2012-05-07 12:27:13 -0700 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. 140436 2012-05-06 14:41:00 -0700 2012-05-06 17:44:28 -0700 Patch bug-85748-20120506174049.patch text/plain 1623 rwlbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMTE2MjM4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDY2MDcwZTY2MzdjNzc5 YTUzMWQ2Y2Y0YTRlODEyMjk3Y2Q4M2Y2ZS4uZTExYzlkMGM4Mjg5YzZiMmYxMjRlN2JiNjBlNzY2 MGIyNjViMWY4NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEyLTA1LTA2ICBSb2Ig QnVpcyAgPHJidWlzQHJpbS5jb20+CisKKyAgICAgICAgSW5saW5lQm94OjpzZXRIYXNCYWRQYXJl bnQgc2hvdWxkIGJlIGRlYnVnIG9ubHkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTg1NzQ4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgRG8gbm90IHByb3ZpZGUgSW5saW5lQm94OjpzZXRIYXNCYWRQYXJlbnQg aW4gcmVsZWFzZSBidWlsZHMuCisKKyAgICAgICAgKiByZW5kZXJpbmcvSW5saW5lQm94Lmg6Cisg ICAgICAgIChJbmxpbmVCb3gpOgorICAgICAgICAoV2ViQ29yZSk6CisgICAgICAgIChXZWJDb3Jl OjpJbmxpbmVCb3g6OnNldEhhc0JhZFBhcmVudCk6CisKIDIwMTItMDUtMDYgIEtldmluIE9sbGl2 aWVyICA8a2V2aW5vQHRoZW9sbGl2aWVycy5jb20+CiAKICAgICAgICAgW3d4XSBVbnJldmlld2Vk LiBCdWlsZCBmaXhlcyBmb3IgcmVjZW50IHRydW5rIGNoYW5nZXMuCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9yZW5kZXJpbmcvSW5saW5lQm94LmggYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvSW5saW5lQm94LmgKaW5kZXggYzRmYzJjMzBjNjkxY2IwMTE0N2U0N2UxZGViMTc2MDE4NjBi NWMwOC4uMmVkOWNkNDBiNDM3MWMzOGE0YmY4MzlhZTJiMmJiN2Q0YTYzNDkxNSAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL0lubGluZUJveC5oCisrKyBiL1NvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9JbmxpbmVCb3guaApAQCAtMjY1LDcgKzI2NSw5IEBAIHB1YmxpYzoKICAg ICAvLyB2aXNpYmxlTGVmdEVkZ2UsIHZpc2libGVSaWdodEVkZ2UgYXJlIGluIHRoZSBwYXJlbnQn cyBjb29yZGluYXRlIHN5c3RlbS4KICAgICB2aXJ0dWFsIGZsb2F0IHBsYWNlRWxsaXBzaXNCb3go Ym9vbCBsdHIsIGZsb2F0IHZpc2libGVMZWZ0RWRnZSwgZmxvYXQgdmlzaWJsZVJpZ2h0RWRnZSwg ZmxvYXQgZWxsaXBzaXNXaWR0aCwgYm9vbCYpOwogCisjaWZuZGVmIE5ERUJVRwogICAgIHZvaWQg c2V0SGFzQmFkUGFyZW50KCk7CisjZW5kaWYKIAogICAgIGludCBleHBhbnNpb24oKSBjb25zdCB7 IHJldHVybiBtX2JpdGZpZWxkcy5leHBhbnNpb24oKTsgfQogCkBAIC00MTgsMTIgKzQyMCwxMiBA QCBpbmxpbmUgSW5saW5lQm94Ojp+SW5saW5lQm94KCkKIH0KICNlbmRpZgogCisjaWZuZGVmIE5E RUJVRwogaW5saW5lIHZvaWQgSW5saW5lQm94OjpzZXRIYXNCYWRQYXJlbnQoKQogewotI2lmbmRl ZiBOREVCVUcKICAgICBtX2hhc0JhZFBhcmVudCA9IHRydWU7Ci0jZW5kaWYKIH0KKyNlbmRpZgog CiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCiAK